Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iot-dev): Fix potential deadlock when closing while reconnecting #1393

Merged
merged 1 commit into from
Oct 14, 2021
Merged

fix(iot-dev): Fix potential deadlock when closing while reconnecting #1393

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 23 additions & 31 deletions device/iot-device-client/src/main/java/com/microsoft/azure/sdk/iot/device/DeviceIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,19 @@ private void startWorkerThreads()
this.state = IotHubConnectionStatus.CONNECTED;
}

private void stopWorkerThreads()
{
if (this.sendTaskScheduler != null)
{
this.sendTaskScheduler.shutdown();
}

if (this.receiveTaskScheduler != null)
{
this.receiveTaskScheduler.shutdown();
}
}

/**
* Completes all current outstanding requests and closes the IoT Hub client.
* Must be called to terminate the background thread that is sending data to
Expand All @@ -251,16 +264,6 @@ public void close()
return;
}

if (this.sendTaskScheduler != null)
{
this.sendTaskScheduler.shutdown();
}

if (this.receiveTaskScheduler != null)
{
this.receiveTaskScheduler.shutdown();
}

this.transport.close(IotHubConnectionStatusChangeReason.CLIENT_CLOSE, null);
this.state = IotHubConnectionStatus.DISCONNECTED;
}
Expand Down Expand Up @@ -452,28 +455,17 @@ void registerMultiplexingConnectionStateCallback(IotHubConnectionStatusChangeCal
@Override
public void execute(IotHubConnectionStatus status, IotHubConnectionStatusChangeReason statusChangeReason, Throwable throwable, Object callbackContext)
{
synchronized (this.stateLock)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lock statement was the cause of the issue. The reconnection thread would first grab the reconnectionLock in IotHubTransport.java, then attempt to notify this DeviceIO layer that the transport is reconnecting, so it would need to grab this stateLock, too.

But if the user had initiated a close call already, then this lock was grabbed already by the user's thread. Once the user's thread has grabbed this stateLock, it then attempts to grab the reconnectionLock in IothubTransport.java.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to synchronize on this lock in this callback since it is either called by the user's thread that called open/close (which already grabbed this lock in this deviceIO layer), or it is called by the reconnection thread. When it is called by the reconnection thread, we know that there isn't a close call happening since we have the reconnectionLock in the IotHubTransport layer preventing exactly that.

if (status == IotHubConnectionStatus.DISCONNECTED || status == IotHubConnectionStatus.DISCONNECTED_RETRYING)
{
if (status == IotHubConnectionStatus.DISCONNECTED || status == IotHubConnectionStatus.DISCONNECTED_RETRYING)
{
// No need to keep spawning send/receive tasks during reconnection or when the client is closed
if (this.sendTaskScheduler != null)
{
this.sendTaskScheduler.shutdown();
}

if (this.receiveTaskScheduler != null)
{
this.receiveTaskScheduler.shutdown();
}
}
else if (status == IotHubConnectionStatus.CONNECTED)
{
// Restart the task scheduler so that send/receive tasks start spawning again
this.startWorkerThreads();
}

this.state = status;
// No need to keep spawning send/receive tasks during reconnection or when the client is closed
this.stopWorkerThreads();
}
else if (status == IotHubConnectionStatus.CONNECTED)
{
// Restart the task scheduler so that send/receive tasks start spawning again
this.startWorkerThreads();
}

this.state = status;
}
}