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.

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

Improve session message injection #2117

Merged

Conversation

AntonSmolkov
Copy link
Contributor

Hi there!
I'm trying to build a scalable, persistent MQTT server around RabbitMQ, using the MQTTNet server as a frontend.

While doing this, I found that there is currently no proper way to correlate injected QoS 1 messages with the packets received by the ClientAcknowledgedPublishPacketAsync event. So, there is no way for me to pass the PUBACK further to RabbitMQ's message acknowledgment.

Additionally, I noticed that DeliverApplicationMessageAsync() method may hang indefinitely when MqttPendingMessagesOverflowStrategy.DropNewMessage is used, because in the case of an output buffer overflow, the packetBusItem being awaited is dropped from the queue without the Task's Result or Exception being set.

So, in this PR:

  • I added MqttPublishPacket as a result of message injection methods, to be able to correlate them by packet id later.
  • Fixed the potential infinite wait in the DeliverApplicationMessageAsync() method.
  • Added some tests.

Thanks for the great project, btw!

@AntonSmolkov
Copy link
Contributor Author

@dotnet-policy-service agree

@AntonSmolkov AntonSmolkov force-pushed the improve_session_message_injection branch from 8d5092d to f96c10c Compare December 1, 2024 14:09
Copy link
Collaborator

@chkr1011 chkr1011 left a comment

Choose a reason for hiding this comment

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

Nice feature! But I recommend changing the public API a bit.

@AntonSmolkov AntonSmolkov force-pushed the improve_session_message_injection branch from 1b4cb4c to 2b1c143 Compare December 2, 2024 16:43
@AntonSmolkov AntonSmolkov force-pushed the improve_session_message_injection branch from 841fa67 to 026029c Compare December 3, 2024 13:40
Copy link
Collaborator

@chkr1011 chkr1011 left a comment

Choose a reason for hiding this comment

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

Just a few minor improvements.

@AntonSmolkov AntonSmolkov requested a review from chkr1011 December 6, 2024 06:48
Copy link
Collaborator

@chkr1011 chkr1011 left a comment

Choose a reason for hiding this comment

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

Good work 👍

@AntonSmolkov
Copy link
Contributor Author

@chkr1011 Thanks!

The CI-workflow is not approved yet. It this intentional?

@AntonSmolkov
Copy link
Contributor Author

@chkr1011 Sorry to bother you again, but the CI has failed on the "sign nugets step". :))

@chkr1011 chkr1011 merged commit 27771f0 into dotnet:master Jan 2, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants