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

[mqtt] upgrade mqtt to tokio1 release #4830

Merged
merged 11 commits into from Apr 15, 2021
Merged

Conversation

dmolokanov
Copy link
Contributor

Upgrade mqtt to a new tokio v1.x ecosystem. There are some known issues which will be addressed in upcoming PRs:

  1. tracing log formatter is default
  2. mqtt3::Client has a timer pinned on heap instead of pinned on stack
  3. percent_encoding is outdated
  4. mockall is outdated

@dmolokanov dmolokanov added the area:mqtt MQTT broker label Apr 12, 2021
arsing
arsing previously approved these changes Apr 12, 2021
mqtt/mqtt3/src/client/mod.rs Outdated Show resolved Hide resolved
@@ -16,7 +16,7 @@ where
IoS: super::IoSource,
{
BeginBackOff,
EndBackOff(tokio::time::Delay),
EndBackOff(std::pin::Pin<Box<tokio::time::Sleep>>),
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to avoid the allocation by changing the client code to support State: !Unpin (which would probably include making Client: !Unpin). And for State itself, the pin-project crate makes it easier to access !Unpin data of !Unpin enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did one pass on using pin-project but got lost. I'm planning to do another pass again soon.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it gets hairy when you have to re-assign self to be a different variant, and especially hairy when you have to re-assign self to be a different variant but containing some of the data from the current variant.

@nyanzebra
Copy link
Contributor

Could these changes be tested against a longhaul or some sort of test setup that runs for a time period of several hours? Given the huge leap in Tokio and nuanced changes in some places, maybe some bake time would be helpful? Your call :)

nyanzebra
nyanzebra previously approved these changes Apr 13, 2021
Copy link
Contributor

@nyanzebra nyanzebra left a comment

Choose a reason for hiding this comment

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

As we talked about, let's wait for the results of a longhaul to make sure there are no 'gotchas' in our upgrade of tokio before merge :)

arsing
arsing previously approved these changes Apr 13, 2021
mqtt/mqtt-bridge/src/messages.rs Show resolved Hide resolved
mqtt/mqtt-bridge/src/messages.rs Outdated Show resolved Hide resolved
mqtt/mqtt-broker/src/connection/packet.rs Show resolved Hide resolved
mqtt/mqtt-broker/tests/compliance.rs Outdated Show resolved Hide resolved
mqtt/mqtt3-fuzz/Cargo.toml Show resolved Hide resolved
@@ -1,4 +1,4 @@
//! `tokio::time::delay_for` and `tokio::time::delay_until` has a bug that
//! `tokio::time::sleep` and `tokio::time::delay_until` has a bug that
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! seems panic was fixed recently. but still

The maximum duration for a sleep is 68719476734 milliseconds (approximately 2.2 years).

source: https://docs.rs/tokio/1.5.0/tokio/time/fn.sleep.html

Copy link
Contributor

Choose a reason for hiding this comment

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

2.2y is not enough for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for test purposes I made the one for 10y and it caused some problems for me previously.

mqtt/mqttd/src/tracing/edgehub.rs Show resolved Hide resolved
Copy link
Contributor

@vadim-kovalyov vadim-kovalyov left a comment

Choose a reason for hiding this comment

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

🚀

@kodiakhq kodiakhq bot merged commit afb8f54 into Azure:master Apr 15, 2021
@dmolokanov dmolokanov deleted the tokio1-upgrade branch April 15, 2021 02:00
ggjjj pushed a commit to ggjjj/iotedge that referenced this pull request Jul 22, 2021
Upgrade mqtt to a new `tokio v1.x` ecosystem. There are some known issues which will be addressed in upcoming PRs:
1. tracing log formatter is default
2. `mqtt3::Client` has a timer pinned on heap instead of pinned on stack
3. `percent_encoding` is outdated
4. `mockall` is outdated
damonbarry pushed a commit to damonbarry/iotedge that referenced this pull request Apr 15, 2022
Upgrade mqtt to a new `tokio v1.x` ecosystem. There are some known issues which will be addressed in upcoming PRs:
1. tracing log formatter is default
2. `mqtt3::Client` has a timer pinned on heap instead of pinned on stack
3. `percent_encoding` is outdated
4. `mockall` is outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants