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.
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!)
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.
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.
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
The goroutine is created in Observable.On(), but is initially blocked in a select. Trigger() is what "kicks it off" in the sense of unblocking the goroutine, so it can do real work. Apologies that that was unclear.
The observer pattern decouples notifiers from observers. Observers may come and go at any time, and whenever a notification is posted, all the currently registered observers see it. If you demand that all observers are added before the first notification, then your notifier has to know which (or how many) observers are going to register! You've coupled your notifiers and observers, which is exactly what we're trying to avoid.
Here's a concrete example: the event is the user pressing a key. Keypresses often create a new window or change the focus or something, so you can have new observers added in response to a keypress. So a Trigger() leads to an On(). It does make sense!
> ...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.
Seems to only work with a single receiver, in a synchronous manner, in which case I wonder what's the point of using that instead of a simple function call.
One common use case is in workflow systems. For example, you might "wait" for a bunch of files to exist before kicking off a job. This could be implemented as a bunch of "subjects" notifying when the files exist, and then the "observer" taking action when all of its subjects notify.
It is also used in UI frameworks to update visual representation when underlying data changes.
Let's say you want to publish a library that for example logs the global keyboard events. If you want to simply notify any keypress in a really simple manner you can just use myLib.On("keypress", func() {}) and no matter where the "keypress" event gets dispatched you will have always a callback listening it.
This smells to me like a Publish/Subscribe mechanism. [0]
I mean, this would be useful anywhere you want to send events/messages to some number of interested "parties", when you don't necessarily know what or who those parties are going to be ahead of time.
I'm not snarking when I ask this, it's a genuine question: Have you ever done that sort of thing before?
I've used almost this exact pattern (in Go) for a data normalization system. Considered sharing the code, but "never got around to it". What I built actually closely resembled node.js's streams.
Really convenient when you've got various chains of transformations that need to be combined/reordered/dropped while processing streams of data.
I am not reinventing the wheel here I have just tried to add a pattern I really like in other programming languages that I was missing in Go. You can use it however you want, and if you don't like it just use something else.
> Have you ever done that sort of thing before? yes I did that's one of the reasons I had to code go-observable
> yes I did that's one of the reasons I had to code go-observable
I don't mean to sound mean or anything like this, but you do know that this question was directed directly at fiatjaf, and noone else, right? :) [0] I mean, I guess I appreciate the input, but I asked the question because it seemed obvious to me -because I spent several years working with software that made use of Pub/Sub mechanisms- that go-observable was a Pub/Sub mechanism for Go. So... I was wondering if fiatjaf had ever done any such thing in the past.
Obviously, as you are publishing a pub/sub mechanism in a given language it's either because you had need to do it in the past (or you were just doing it for funzies) and are sharing the fruits of your efforts. :)
[0] Or did my comment appear to be a direct descendent of your HN post, rather than a descendent of fiatjaf's question? I've heard that particular bug mentioned once or twice in the past.
Ok sorry I thought the question was referred to me :)
There are many pub/sub modules for go but I made this one because I was missing in some case the asynchronous callbacks that I can use with other languages ( node for example ) but in a really simple way without too many ceremony (probably I am too stupid for other solutions) and also with the benefit of using the async go routines that IMHO are an advantage and not cons like someone else pointed out (performance tests are still needed here)
I was under the impression that this was useful only for GUI programming, but yes, that's a use case. I'm happy to know there are other legit use cases.
Oh yeah! Pub/sub is useful anywhere you have an N message producers and M message consumers (where both N and M are >= 0).
I mean, the way pretty much everyone does GUIs (with the central message pump that pulls external events off of a queue and changes UI state based on those messages) is an entirely reasonable way to think of all systems that use message passing to communicate -e.g. Elixir or Erlang- between loosely coupled parts of the system.
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.