fix(iot-dev): Fix race condition where reconnection logic could carry on after client was closed#1356
Conversation
… on after client was closed If close is called during reconnection, it will block until the current reconnection attempt completes (successfully or not) before closing the connection and marking the local state as closed If reconnection is initiated during a close, it will block until the close call has finished tearing down the connection and marking the local state as closed. After the close call has finished, the reconnection logic will immediately give up since the client was closed
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
drwill-ms and
jamdavi
as code owners
| && transportException != null | ||
| && transportException.isRetryable()) | ||
| { | ||
| if (this.isClosing) |
There was a problem hiding this comment.
This check occurs in the reconnection loop, so if close() is called while this reconnection is already happening, it will block until the latest reconnection attempt completes and the thread exits before the close() call continues.
| throw new TransportException("Open cannot be called while transport is reconnecting"); | ||
| } | ||
|
|
||
| this.isClosing = false; |
There was a problem hiding this comment.
This flag is reset to false every time the transport is opened
| } | ||
|
|
||
| this.cancelPendingPackets(); | ||
| this.isClosing = true; |
There was a problem hiding this comment.
Set the flag outside of the synchronized block so that any currently running reconnection logic knows to give up when this flag is set to true. Then the rest of the close() code is in the synchronization block so that it waits for the reconnection logic to end before it starts
There was a problem hiding this comment.
Sounds like a good code comment
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
|
||
| if (this.taskScheduler != null) | ||
| // Wait until no reconnection logic is taking place | ||
| synchronized (this.reconnectionLock) |
There was a problem hiding this comment.
Having locks within a lock makes me worried about deadlocks. Thoughts?
There was a problem hiding this comment.
Are you referring to the sendThreadLock and receiveThreadLock? Neither of those locks are acquired or modified in the reconnection thread. There is no chance that the reconnection thread holds onto the sendThreadLock or receiveThreadLock while the close() thread holds the reconnectionLock so we're safe from deadlock
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
3 participants
If close is called during reconnection, it will block until the current reconnection attempt completes (successfully or not) before closing the connection and marking the local state as closed
If reconnection is initiated during a close, it will block until the close call has finished tearing down the connection and marking the local state as closed. After the close call has finished, the reconnection logic will immediately give up since the client was closed
#1343