fix(client): Adding graceful shutdown to event loop #2164
Conversation
jamdavi
requested review from
abhipsaMisra,
azabbasi,
drwill-ms and
timtay-microsoft
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Also #2194 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /// <remarks> | ||
| /// The DotNetty Mqtt transport has a graceful shutdown of the event loop that can block the <see cref="DeviceClient.CloseAsync(System.Threading.CancellationToken)"/> method. This timeout allows you to configure how long the shutdown event will wait for before forcefully shutting down the event loop. Change this if you need your application to respond to the CloseAsync command faster. | ||
| /// </remarks> | ||
| public TimeSpan GracefulEventLoopShutdownTimeout { get; set; } = TimeSpan.FromSeconds(1); |
There was a problem hiding this comment.
This was 1 millisecond before. I asked why but the only response I see is changing it to 1 second. Either way, I don't understand how we've chosen a default value and why is 1 second a good default? What are the implications to the user?
There was a problem hiding this comment.
So by default the timeout value is 14 seconds... I can make this a TimeSpan.Zero and then let the customer decide if they want to lower it.
There was a problem hiding this comment.
Same questions about he timeout values selected. It would be helpful to add a reference to the netty specs which define these timeout values (default/recommended etc).
shared/src/ReferenceCounter.cs
Outdated
|
|
||
| private void CreateWithoutLocking(Func<T> objectToCreate) | ||
| { | ||
| if (_obejctToCount == null && _referenceCount <= 0) |
There was a problem hiding this comment.
under what circumstances does _referenceCount go below zero?
There was a problem hiding this comment.
None, now that think of it.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /// Sets the MqttTransport to use a single Event Loop Group. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This setting is to retain legacy behavior that will use a single Event Loop Group which is the thread pool model for DotNetty. If this is set to false a new group will be created per instance of the Device Client. |
There was a problem hiding this comment.
We have 1 instance of mqtt adapter per device client, and we can have 1 instance of device client for a particular device identity. Considering that, what does this mean:
"If this is set to false a new group will be created per instance of the Device Client.", i.e. does this refer to the scenario where we dispose a client instance and reinitialize?
shared/src/ReferenceCounter.cs
Outdated
| /// <typeparam name="T">Any class based object type.</typeparam> | ||
| public class ReferenceCounter<T> where T : class | ||
| { | ||
| private T _obejctToCount; |
| /// | ||
| /// | ||
| /// | ||
| /// This will return a mutable object. It is possible to alter the object state which could cause unexpected behavior. |
There was a problem hiding this comment.
q - What kind of unexpected behavior are we referencing here?
There was a problem hiding this comment.
In general if you have an object that is reference counted there is no way to determine if the object has been changed. For example, an object could be disposed of while there is still a reference count of 1 but we'd have no way to tell and we'd still return the reference to the disposed object.
… clearing the eventloopgroup
|
Closing this as the following fix will be released by DotNetty |
When we create an Mqtt transport handler we set the EventLoopGroup but we do not gracefully shut it down. This leaves a number of TaskSchedulers open.
This is specific to DotNetty (well Netty really) and does not affect Amqp.
In most cases this will not cause a problem if the application closes after client shutdown, but in WPF applications it will cause the main UI thread to hang and never close.
By default this shutdown will take 15 seconds to complete.
This is documented in the Java Netty wiki.
This will address #2163 and #2194