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

Fixing the naive implementation of sleep #1523

Merged
merged 1 commit into from
Dec 27, 2023
Merged

Conversation

msabansal
Copy link
Contributor

Address Issue highlighted in #1515 in the naive implementation.

Delegates to tokio's sleep if tokio feature is turned on

@demoray
Copy link
Contributor

demoray commented Dec 21, 2023 via email

@msabansal
Copy link
Contributor Author

msabansal commented Dec 21, 2023

Instead of creating the atomic during poll, why not create it when creating the Sleep object?

Can do that and improve. Wanted to see if this approach works. I had the Issue opened to hope and have the discussion there but since it didnt move i thought i would just open a PR. Also initial implementation thought was to launch thread on first poll

@demoray
Copy link
Contributor

demoray commented Dec 21, 2023 via email

@msabansal
Copy link
Contributor Author

Can do that and improve. Wanted to see if this approach works. I had the Issue opened to hope and have the discussion there but since it didnt move i thought i would just open a PR.
Many (most) of us take much of December off, hence the delay. I'm responding via email as I'm away from work. I would also like to split off the tokio part as it's own off-by-default feature (and PR), and keep this pr to just fixing the bug you're experiencing.

Also out of office but getting to play around with things :). Thanks for replying and reviewing the PR. Will split it in that manner

@msabansal
Copy link
Contributor Author

Made the tokio sleep alternative opt-in disabled by default. The naive implementation does a let of work in the poll as it needs to grab the wake context to wake the blocked task.

@@ -62,7 +62,8 @@ hmac_openssl = ["dep:openssl"]
test_e2e = []
azurite_workaround = []
xml = ["quick-xml"]
tokio-fs = ["tokio/fs", "tokio/io-util"]
tokio-fs = ["tokio/fs", "tokio/sync", "tokio/io-util"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed to the tokio-fs feature that requires tokio/sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses mutex which is behind sync feature flag. It just didn't compile. Should fix ci tests as well

@demoray demoray merged commit 1160d50 into Azure:main Dec 27, 2023
19 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.

None yet

2 participants