rethrow MqttClientUnexpectedDisconnectReceivedException when Publish … #1974
Conversation
…errors out because server disconnects the client and QoS is 0
|
@chkr1011 fyi |
Source/MQTTnet/Client/MqttClient.cs
Outdated
| } | ||
| catch (Exception) | ||
| { | ||
| if (_unexpectedDisconnectPacket != null) |
There was a problem hiding this comment.
Does this only apply for QoS 0? I am wondering if this code must be also used for QoS 1 and 2?
There was a problem hiding this comment.
I think it only applies to QoS 0 because for QoS 1, the client actually registers an awaitable and wait for the response packet, so it already throws an exception with the error code as of today. But for QoS 0, the client does not register any awaitable. I guess this is by design because QoS 0 should not expect any response from server. But QoS 1 can always expect some response, whether its a PUBACK or DISCONNECT
|
@chkr1011 any thoughts on the concern I raised? Or if not, can you approve this PR? |
Source/MQTTnet/Client/MqttClient.cs
Outdated
| return MqttClientResultFactory.PublishResult.Create(null); | ||
| if (localUnexpectedDisconnectPacket != null) | ||
| { | ||
| throw new MqttClientUnexpectedDisconnectReceivedException(localUnexpectedDisconnectPacket); |
There was a problem hiding this comment.
I would pass the actual exception to the inner exception so that it is still accessible for the user.
|
I will add the breaking change to the release notes. Breaking changes are happening from time to time and cannot be avoided. In this case we still get an exception. Only the the type is different now. I added a comment to the code. |
|
@fazho I made the change myself and also updated the release notes and added a comment. Please let me know if you agree with the changes (especially the comment). After that I will merge the PR. |
|
@chkr1011 your changes look good to me. I just made another small change, setting the default inner exception as null in the constructor of MqttClientUnexpectedDisconnectReceivedException, so that if anyone explicitly creates an MqttClientUnexpectedDisconnectReceivedException object in their code (e.g., someone might mock the MQTTClient and create this exception in their unit tests), our changes in this PR will not break it. I know this will be a breaking change either way, but just try to make the breaking part minimum. |
|
@chkr1011 feel free to revert my latest change if it does not make sense to you. I am ok either way. |
rethrow MqttClientUnexpectedDisconnectReceivedException when QoS is 0 and Publish errors out because server disconnects the client.