Unregister ConnectionWaiter listeners once connected#652
Unregister ConnectionWaiter listeners once connected#652QuintinWillison merged 1 commit intomainfrom
Conversation
…n token changes) when it falls out of scope. Also refactors the code that calls this instance to make it more readable (avoiding use of continue, for example).
| * Remove this ConnectionWaiter as a connection listener. | ||
| */ | ||
| private void close() { | ||
| // This method is explicitly not synchronized. There may be a case for this in the |
There was a problem hiding this comment.
In this repo we've tended to use /* .... */ comment style
There was a problem hiding this comment.
For interface commentary I would agree, however within a method implementation I tend to prefer // as it's more compact - typically for single line comments but also for multi-line. I should document this somewhere.
| private void close() { | ||
| // This method is explicitly not synchronized. There may be a case for this in the | ||
| // future, however its addition is designed to be lightweight with minimal impact. | ||
| if (closed) { |
There was a problem hiding this comment.
I think adding synchronized would be safe, but it this case it happens not to be necessary; if in the race the .off() is called twice, the second call would have no effect. Also, the calling code cannot end up calling close() more than once.
| if (closed) { | ||
| return; | ||
| } | ||
| closed = true; |
There was a problem hiding this comment.
I feel closed=true can be set after connection.off(this);, that way it's logically closed after removing it from connection
There was a problem hiding this comment.
Thanks for the suggestion, but I still prefer it this way around.
| waitingForConnected = false; | ||
| break; | ||
|
|
||
| case connecting: |
There was a problem hiding this comment.
Not sure, but if it's connecting, it can transition to connected right ... that way it shouldn't break and just continue to check if the next state is connected
There was a problem hiding this comment.
Maybe we can add explicit continue ... WDYT
There was a problem hiding this comment.
I'm not quite sure what you mean, as this whole switch statement is validating a single state. So, connected is transitory and connected will get encountered on a subsequent iteration of the while.
|
Thanks all for the prompt reviews. 😁 |
4 participants
Fixes #651.
This is a relatively tentative fix in which I'm fairly confident due to the isolated and internal nature of the fix.
The fundamental issue was that we were not removing
ConnectionWaiterinstances as connection listeners, however I've also refactored the code a little for clarity.There is probably plenty more I could have refactored but I wanted to keep the impact of this change minimal as we're aiming to get this out quickly as a bug fix.