Client: Tolerate late acknowledgement packets (#2078, #2079)#2247
Merged
chkr1011 merged 2 commits intoJun 16, 2026
Merged
Conversation
When a publish, subscribe or unsubscribe request was cancelled or timed out client-side, the corresponding awaitable was removed from the packet dispatcher. A subsequent PUBACK / PUBCOMP / SUBACK / UNSUBACK arriving from the broker then fell through to the default case and raised an MqttProtocolViolationException, which forced a disconnect. Under high publish load (issue dotnet#2079) and from multiple threads (issue dotnet#2078) this manifested as sporadic connection drops. Match these acknowledgement packets explicitly and log a warning when no awaiter is registered, instead of treating it as a protocol violation. The connection is now preserved. Also documents the thread-safety guarantees of IMqttClient: publish, subscribe, unsubscribe and ping operations are safe to invoke concurrently. Connect / disconnect must still be coordinated.
Contributor
Author
|
@dotnet-policy-service agree |
…col-violation # Conflicts: # Source/ReleaseNotes.md
chkr1011
approved these changes
Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2078
Fixes #2079
Problem
When a
PublishAsync/SubscribeAsync/UnsubscribeAsynccall is cancelled or times out client-side, its awaitable is removed fromMqttPacketDispatcher. If the broker''s matchingPUBACK/PUBCOMP/SUBACK/UNSUBACKarrives slightly later it falls through thedefaultcase inMqttClient.TryProcessReceivedPacket, raisesMqttProtocolViolationException("Received packet ''...'' at an unexpected time."), and tears down the connection.This surfaced as:
MqttProtocolViolationExceptionon a tiny fraction of QoS 1 publishes when sending high volumes (~100k messages).MqttClientappearing unsafe under concurrent publishes from multiple threads.Why #2078 is the same root cause (not a missing lock)
The two issues look different but share one bug. The client was already correctly synchronised at every relevant layer; concurrency just made the late-ack race far more likely to be hit.
What was already protected:
MqttPacketIdentifierProvider.GetNextPacketIdentifierlock (_syncRoot)— each caller gets a unique IDMqttPacketDispatcher.AddAwaitablelock (_waiters)MqttChannelAdapter.SendPacketAsyncawait _syncRoot.EnterAsync(...)(AsyncLock) — wire bytes never interleaveMqttPacketAwaitable<T>MqttPacketDispatcher.TryDispatchmatches by IDSo two threads calling
PublishAsyncsimultaneously was fine at the locking level. What broke under load was this sequence:PublishAsync(message, ctsA.Token)with a tight timeout.Nis registered, the PUBLISH goes on the wire.ctsAfires before the broker''s PUBACK arrives - the awaitable is removed.Narrives.MqttProtocolViolationException→ connection torn down.That is why users perceived "the client breaks when used from multiple threads": one caller''s cancellation killed everyone else''s connection. The same race produced #2079 in a single-threaded high-volume scenario, where a small fraction of publishes get cancelled by their own timeout.
Adding a
SemaphoreSlimaroundPublishAsyncwould have been the wrong fix - it would serialise publishes (hurting throughput) without addressing the actual bug, since the late-ack race exists regardless of how many threads are involved.Fix
MqttClient.TryProcessReceivedPacketnow matchesPUBACK/PUBCOMP/SUBACK/UNSUBACKexplicitly. If no awaiter is registered the packet is logged as a warning and dropped, rather than treated as a protocol violation. The connection is preserved.PUBREC/PUBRELalready had spec-compliant handling for unknown identifiers and are unchanged.IMqttClientnow documents its concurrency contract:PublishAsync,SubscribeAsync,UnsubscribeAsyncandPingAsyncare safe to invoke concurrently from multiple threads.ConnectAsync/DisconnectAsyncmust still be coordinated on a single thread.Tests
Two new tests in
MqttClient_Tests:Cancelled_Publish_Does_Not_Disconnect_Client(AtLeastOnce,ExactlyOnce) - usesInterceptingPublishAsyncto delay the broker''s ack, cancels the publish, and asserts the client stays connected and remains usable. Verified to fail on the unfixed code withMqttProtocolViolationException.Concurrent_Publish_From_Multiple_Threads(AtMostOnce,AtLeastOnce,ExactlyOnce) - 8 threads x 250 messages, asserts no disconnect and that every message is delivered to a subscriber.Full suite: 487 / 487 passed locally on
net8.0.Release notes
Entry added to
Source/ReleaseNotes.md.