Skip to content

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.

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
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