[FIXED] Dispatching of asynchronous callbacks #369
Conversation
This is a follow up on #365. Working on another issue and running tests, I still had a race on the original ach or a panic on send to close channel. Move from a channel to a linked list with dedicated lock. Signed-off-by: Ivan Kozlovic <[email protected]>
| // Make sure that library is not calling push with nil function, | ||
| // since this is used to notify the dispatcher that it should stop. | ||
| if !close && f == nil { | ||
| panic("pushing a nil callback") |
There was a problem hiding this comment.
Do we have any other panics in our library?
There was a problem hiding this comment.
This is not user accessible. This is to protect us, writers of the library. That is, to catch a case where we would add some code that happen to call push() with a nil callback (if we were not checking nc.Opts.).
nats.go
Outdated
| nc.mu.Unlock() | ||
| } | ||
| // Returns an initialized asyncCallbacksHandler object | ||
| func newAsyncCallbacksHandler() *asyncCallbacksHandler { |
There was a problem hiding this comment.
Seems to be only called once and very short, maybe just inline?
There was a problem hiding this comment.
actually called in a test too.
| if !close && f == nil { | ||
| panic("pushing a nil callback") | ||
| } | ||
| cb := &asyncCB{f: f} |
There was a problem hiding this comment.
if close is true does f have to be nil?
There was a problem hiding this comment.
I originally used a closed boolean in the structure but thought that I could do without and pass nil as the last function to indicate close event. But yes, f will be nil if close is true. Again, this is not user accessible and only close() will call pushOrClose with nil, true
nats.go
Outdated
| ac.head = cb | ||
| } | ||
| ac.tail = cb | ||
| ac.cond.Signal() |
There was a problem hiding this comment.
For close scenarios I like this to be a Broadcast instead.
There was a problem hiding this comment.
Sorry, but there is only one go routine ever calling Wait() so Signal() is enough.
There was a problem hiding this comment.
I know that is the desired state. But kind of like your panic thought best to kick out any and all IMO.
test/conn_test.go
Outdated
| ncp.Close() | ||
|
|
||
| // Give a chance for the go routine to stop | ||
| time.Sleep(150 * time.Millisecond) |
There was a problem hiding this comment.
Be good if this was deterministic instead of time based.
There was a problem hiding this comment.
I could add specific wait group in the struct and use that for the test, but I feel it is a bit too much. What I could do is repeat the test with a little delay in case of failure and fail the test after few attempts. I will do that.
- Use Broadcast() instead of Signal() - Presence of asyncCBDispatcher go routine less sensitive to timing. Signed-off-by: Ivan Kozlovic <[email protected]>
nats.go
Outdated
| @@ -1605,7 +1605,7 @@ func (ac *asyncCallbacksHandler) pushOrClose(f func(), close bool) { | |||
| ac.head = cb | |||
| } | |||
| ac.tail = cb | |||
| ac.cond.Signal() | |||
| ac.cond.Broadcast() | |||
There was a problem hiding this comment.
I meant only on close condition. Signal as default is fine.
There was a problem hiding this comment.
Done.. but to be honest, this is some code that one will scratch their head in few months/years and ask themselves: why is that? ;-)
If there is a chance that we have multiple go routines calling Wait() then it should always be Broadcast(), but that would defeat the purpose of this code anyway since it is meant to ensure that callbacks are dispatched in order.
Signed-off-by: Ivan Kozlovic <[email protected]>
|
LGTM |
Signed-off-by: Ivan Kozlovic <[email protected]>
This is a follow up on #365. Working on another issue and running
tests, I still had a race on the original ach or a panic on send
to close channel.
Move from a channel to a linked list with dedicated lock.
Signed-off-by: Ivan Kozlovic [email protected]