Handle Client.Close on cloned clients #893
Conversation
cretz
requested review from
mfateev,
Sushisource,
alexshtin,
Spikhalskiy,
bergundy and
mmcshane
as code owners
| workflowClient := client.(*WorkflowClient) | ||
|
|
||
| // Confirm there is 1 unclosed client | ||
| require.EqualValues(t, 1, *workflowClient.unclosedClients) |
There was a problem hiding this comment.
technically should use atomic.LoadInt32, if you modify it with atomics, I think
There was a problem hiding this comment.
This is in a test case. I can trust my lack of race issues in my own test case.
There was a problem hiding this comment.
I would do it just for consistency's sake. It's supposed to be an atomic int, it should be manipulated atomically even if Go lets you not do that. Trivial amount of extra typing.
There was a problem hiding this comment.
I also access the pointers directly in a non-atomic way to compare they point to the same spot. But I will make the change.
| // set it to a new pointer of max to prevent decrementing on repeated Close | ||
| // calls to this client. If the count has not reached zero, this close call is | ||
| // ignored. | ||
| if wc.unclosedClients != nil { |
There was a problem hiding this comment.
I cannot trust all places that create a client will set this. That includes tests and other code paths.
| if err := wc.conn.Close(); err != nil { | ||
| wc.logger.Warn("unable to close connection", tagError, err) | ||
|
|
||
| if wc.conn != nil { |
There was a problem hiding this comment.
Test cases w/ mocks. This logic has not changed, I just moved the condition from the top of the function to down here.
|
This is reliably causing integration tests to go from 2 mins to 3. I am investigating... EDIT: This was due to an unclosed client causing a temporary goroutine leak causing a timeout |
What was changed
Use a counter to make sure only the last
Close()of a shared-connection set of clients applies.Checklist