MQTTnet
MQTTnet copied to clipboard
dotnet
add a Disconnecting event to MqttClient
What is the purpose of the new event? To me it looks like a copy of the already existing "Disconnected" event but it is fired before doing the actual disconnect!? Do you have a sample what it is used for?
It's the same reason why we have a Connecting event, which happens right after the connection status is transitioned into Connecting but the work of connecting to the server has not started yet, even though we have a Connected event.
Disconnected happens after everything is done, while Disconnecting happens before that. That is the difference.
With this new event, I can get an accurate reason code from the DISCONNECT packet when PublishAsync() errors out, because the newly added DISCONNECTING event is invoked synchronously before all the resources of the client are released. As a contrast, the Disconnected event happens afterwards, which means that the resources of the client are released first. And depending on the timing, PublishAsync() could error out before or after Disconnected event happens, in which I might not get the reason code when the error occurs.
Here is an example: https://github.com/dotnet/MQTTnet/issues/1963. With the new event, I can set the value of mqttClientPassiveDisconnectReason in the Disconnecting event's callback, so when the exception is thrown, I can know for sure that the reason code from the DISCONNECT packet is assigned to the variable.
@chkr1011
Basically it guarantees us an early access to the reason code in the DISCONNECT packet when server disconnects the client.
Thank you for your explanation. In my opinion there should be no new event at all because you have to change to do anything to avoid disconnecting etc.
We might consider exposing the information in the Publish as well. Lets imagine the publish does not work and gets an error. We might add a property to the disconnection information so that you have it in the catch clause of the Publish method. Then you don't need to synchronize the data from one event handler to a field and access that field from a different event handler. That do you think about that?
@chkr1011 that's even better. If I understand correctly, you are saying we should catch the exception from Publish, and re-throw a specific exception type (for example, MqttClientUnexpectedDisconnectReceivedException) that could contain the info from the disconnect packet. There's only 1 problem I can think of - We are making a breaking change by re-throwing a new exception. In my example, when QoS is 0, what the client throws now, like TaskCanceledException or MqttCommunicationException, will be replaced by whatever the new exception we decide to throw to contain the disconnect info. Is that acceptable? Meanwhile with this new event, we do not need to make any breaking changes.
@chkr1011 I sent out another PR: https://github.com/dotnet/MQTTnet/pull/1974. Could you let me know if this meets your expectation?
@fazho Do we still need this feature? Am I wrong or is the pull request #1974 the solution for this?
@chkr1011 No we don't need this PR given the new one: https://github.com/dotnet/MQTTnet/pull/1974