client: stop the background thread before closing #423
Conversation
| @@ -151,6 +152,10 @@ func (client *client) Close() error { | |||
| return ErrClosedClient | |||
| } | |||
|
|
|||
| // shutdown and wait for the background thread before we take the lock, to avoid races | |||
| close(client.closer) | |||
| <-client.closed | |||
There was a problem hiding this comment.
The job still gets started, it just shuts down immediately (see lines 536-540). The close(client.closed) call is deferred, so if it returns immediately the channel still gets closed.
There was a problem hiding this comment.
Just saw that, thanks! deleting my comments.
There was a problem hiding this comment.
It will, however, deadlock if the client calls Close() on itself because of an error during initialization. So something still to fix.
Otherwise, in the rare case where a background metadata refresh coincided exactly with a call to `Client.Close()` there was a race where we might have potentially tried to write to a nil map. Also add a test for this. Fixes #422.
To catch if anybody accidentally breaks client startup/shutdown in this configuration.
eapache
force-pushed
the
client-shutdown-race
branch
from
9c715b9 to
52af5d9
Compare
|
Approach looks good. Too bad the test needs |
|
I'm not as concerned about this test, since the sleeps are mainly to accommodate the timer of the background thread (whose value we control, but which can't just be 0) rather than to ensure things happen in a very particular order. I don't expect this test to be flaky with respect to future changes. |
client: stop the background thread before closing
Otherwise, in the rare case where a background metadata refresh coincided
exactly with a call to
Client.Close()there was a race where we might havepotentially tried to write to a nil map. Also add a test for this.
Fixes #422.
@Shopify/kafka @epsniff