4 ms·

This package is using threads but not safely. A call to Trigger kicks off the `listen` function in a background thread, which accesses a map and walks over a li

This package is using threads but not safely. A call to Trigger kicks off the `listen` function in a background thread, which accesses a map and walks over a list of callbacks. If you then call On() or Off() immediately after Trigger, that map and/or list gets modified while `listen` is walking it. This is a serious problem. It's great that there's tests, but all of the tests follow the same pattern: On and then Trigger. It's the reverse order that's busted.

JamesMcMinn 11y ago
To add to this slightly, The race detector built into Go is fantastic and very easy to use (simply "go build -race")
mitchellh 11y ago
The above is completely correct. In addition to this, there is a lot of `reflect` usage. This in and of itself isn't really a problem, but this will really slow down the usage and I imagine there are relatively high performance use cases for the observer pattern. Not trying to discourage you or anything! It is great that you built something and made it available for the world. Just trying to give some hints from someone that has worked with Go a lot (parent has as well!)
- gianlucaguarini 11y ago
  Ok thanks I will try to add some performance tests
- zippoxer 11y ago
  Reflect usage is a slow down, but you know what? Still much faster than most dynamic languages
elithrar 11y ago
As of Go 1.6 the concurrent map access will also forcibly crash the program: https://tip.golang.org/doc/go1.6#runtime https://tip.golang.org/doc/go1.6#runtime
hesdeadjim 11y ago
I love Go and use it daily but the sentiment that it somehow makes multithreaded programming "easy" is greatly over-stated. Channels are easy to deadlock or leave dangling in goroutines and any usage of more standard synchronization primitives have all the same issues as any other language. That said, built in race condition checking is a great feature and tests should always be run with it. Unfortunately this library would not gain any benefit from it without tests that actually execute code concurrently.
- Matthias247 11y ago
  Totally agree with you. Multithreaded programming is never easy, neither in Go nor in any other multithreaded language. Erlang is probably the safest because of only immutable data, but you can still deadlock there. The Observer pattern which is shown here can be especially tricky. Once you register a callback for an event you don't necessarily know from which thread the callback will be called. Some libraries will fire callbacks from a worker thread, other might marshal them to some kind of main thread, etc. If callbacks are fired from multiple threads then event ordering won't be guaranteed and you probably need locking in the receiver (be aware of deadlocks). The next tricky thing is cancellation/unsubscription. In most multithreaded implementations you might still get notifications shortly after unsubscribe during special timing conditions if the thread that fires the notification has not yet processed the unsubscription (or operates on a copied and outdated subscriber list). A singlethreaded runtime (like node) makes reasoning around this much easier. And you need a lot of less extra safeguarding code to care for these race conditions.
aerioux 11y ago
@all the commenters above - maybe submit a PR if you so wish? Not to make the library better per say, but to help the author -- learning and all
- gianlucaguarini 11y ago
  pull requests and issues on the repo are really welcome!
gianlucaguarini 11y ago
A call to Trigger does not call start go routine https://github.com/GianlucaGuarini/go-observable/blob/master/observable.go#L37-L42 https://github.com/GianlucaGuarini/go-observable/blob/master.... I agree it can be nice to add a check to skip Trigger calls on non-exsisting channles. I don't know how you use the observable patterns but normally you first set you listeners and then you trigger the events, the other way around makes no sense
- simoncion 11y ago
  > ...but normally you first set you listeners and then you trigger the events, the other way around makes no sense Perhaps I might have misunderstood what you're saying, so bear with me: What about the case where you have two, completely independent processes: a event data Publisher, and a Subscriber proxy that proxies Subscription and Event transmissions between your Go program and the rest of the world via -I don't know- Websockets or plain TCP/IP or something. When your Go program starts, you're likely to want start up your publisher immediately. As your program generates reportable events, it'll call Trigger even if noone has subscribed to those events, right? I mean, the whole point of a pub/sub mechanism is for the event producer(s) to not care at all about any of the event consumers... so it sounds to me like the case where Trigger will be called before anyone has called On is common. [0] Anyway. A general thought about safe software design: It's still important to ensure that unusual or incorrect usage of the public parts of an API fail safe whenever reasonably possible. Even the most conscientious programmers will have a brain fart and fail to remember the order in which API functions should be called. If you can detect improper usage, ideally you should signal an error (whether by return value or exception) and leave everything behind the API untouched. Trashing the list of subscribers because someone called On or Off immediately after calling Trigger is surprising and should be something that can't happen. Anyway, good luck with your project. Think of fun and exciting ways that boneheads like me could break it, and make it so that we can't break it like that! :) [0] Back when I used pub/sub mechanisms to attach networked controls to remote computerized systems, it was absolutely a requirement for each of those systems to run the same regardless of whether there were 0, 1, or 100 sets of controls hooked to them.
  - 11y ago

JamesMcMinn 11y ago

To add to this slightly, The race detector built into Go is fantastic and very easy to use (simply "go build -race")

mitchellh 11y ago

The above is completely correct. In addition to this, there is a lot of `reflect` usage. This in and of itself isn't really a problem, but this will really slow down the usage and I imagine there are relatively high performance use cases for the observer pattern. Not trying to discourage you or anything! It is great that you built something and made it available for the world. Just trying to give some hints from someone that has worked with Go a lot (parent has as well!)

gianlucaguarini 11y ago
Ok thanks I will try to add some performance tests
zippoxer 11y ago
Reflect usage is a slow down, but you know what? Still much faster than most dynamic languages

elithrar 11y ago

As of Go 1.6 the concurrent map access will also forcibly crash the program: https://tip.golang.org/doc/go1.6#runtime https://tip.golang.org/doc/go1.6#runtime

hesdeadjim 11y ago

I love Go and use it daily but the sentiment that it somehow makes multithreaded programming "easy" is greatly over-stated. Channels are easy to deadlock or leave dangling in goroutines and any usage of more standard synchronization primitives have all the same issues as any other language. That said, built in race condition checking is a great feature and tests should always be run with it. Unfortunately this library would not gain any benefit from it without tests that actually execute code concurrently.

Matthias247 11y ago
Totally agree with you. Multithreaded programming is never easy, neither in Go nor in any other multithreaded language. Erlang is probably the safest because of only immutable data, but you can still deadlock there. The Observer pattern which is shown here can be especially tricky. Once you register a callback for an event you don't necessarily know from which thread the callback will be called. Some libraries will fire callbacks from a worker thread, other might marshal them to some kind of main thread, etc. If callbacks are fired from multiple threads then event ordering won't be guaranteed and you probably need locking in the receiver (be aware of deadlocks). The next tricky thing is cancellation/unsubscription. In most multithreaded implementations you might still get notifications shortly after unsubscribe during special timing conditions if the thread that fires the notification has not yet processed the unsubscription (or operates on a copied and outdated subscriber list). A singlethreaded runtime (like node) makes reasoning around this much easier. And you need a lot of less extra safeguarding code to care for these race conditions.

aerioux 11y ago

@all the commenters above - maybe submit a PR if you so wish? Not to make the library better per say, but to help the author -- learning and all

gianlucaguarini 11y ago
pull requests and issues on the repo are really welcome!

gianlucaguarini 11y ago

A call to Trigger does not call start go routine https://github.com/GianlucaGuarini/go-observable/blob/master/observable.go#L37-L42 https://github.com/GianlucaGuarini/go-observable/blob/master.... I agree it can be nice to add a check to skip Trigger calls on non-exsisting channles. I don't know how you use the observable patterns but normally you first set you listeners and then you trigger the events, the other way around makes no sense

simoncion 11y ago
> ...but normally you first set you listeners and then you trigger the events, the other way around makes no sense Perhaps I might have misunderstood what you're saying, so bear with me: What about the case where you have two, completely independent processes: a event data Publisher, and a Subscriber proxy that proxies Subscription and Event transmissions between your Go program and the rest of the world via -I don't know- Websockets or plain TCP/IP or something. When your Go program starts, you're likely to want start up your publisher immediately. As your program generates reportable events, it'll call Trigger even if noone has subscribed to those events, right? I mean, the whole point of a pub/sub mechanism is for the event producer(s) to not care at all about any of the event consumers... so it sounds to me like the case where Trigger will be called before anyone has called On is common. [0] Anyway. A general thought about safe software design: It's still important to ensure that unusual or incorrect usage of the public parts of an API fail safe whenever reasonably possible. Even the most conscientious programmers will have a brain fart and fail to remember the order in which API functions should be called. If you can detect improper usage, ideally you should signal an error (whether by return value or exception) and leave everything behind the API untouched. Trashing the list of subscribers because someone called On or Off immediately after calling Trigger is surprising and should be something that can't happen. Anyway, good luck with your project. Think of fun and exciting ways that boneheads like me could break it, and make it so that we can't break it like that! :) [0] Back when I used pub/sub mechanisms to attach networked controls to remote computerized systems, it was absolutely a requirement for each of those systems to run the same regardless of whether there were 0, 1, or 100 sets of controls hooked to them.
- 11y ago