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

Enter tokio context outside of a future #21

Closed
jplatte opened this issue Oct 10, 2023 · 8 comments · Fixed by #22
Closed

Enter tokio context outside of a future #21

jplatte opened this issue Oct 10, 2023 · 8 comments · Fixed by #22

Comments

@jplatte
Copy link
Contributor

jplatte commented Oct 10, 2023

Sometimes, non-poll methods expect to be able to spawn tasks. One common case is Drop implementations of types that want to do some async cleanup. Would you accept PRs for any of the following?

  • enter_tokio_context + TokioEnterGuard wrapping TOKIO1.enter()
  • impl Drop<T> for Compat<T> that enters the tokio context while dropping the inner value (requires some unsafe for ManuallyDrop, I think)
@notgull
Copy link
Member

notgull commented Oct 10, 2023

I'm more of a fan of the second option. You shouldn't need ManuallyDrop, just wrap the type in Option and take it out to drop it. I would accept this PR.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 10, 2023

Yes, Option works too but has unnecessary overhead.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 10, 2023

Just realized that Compat uses pin_project! for its definition, which prohibits custom Drop impls. So the Drop impl is only an option if that safe abstraction is replaced with hand-checked unsafe.

@taiki-e
Copy link
Collaborator

taiki-e commented Oct 10, 2023

pin_project! for its definition, which prohibits custom Drop impls.

pin-project-lite supports custom Drop impl: taiki-e/pin-project-lite#25

@notgull
Copy link
Member

notgull commented Oct 10, 2023

Yes, Option works too but has unnecessary overhead.

I doubt the overhead is noticeable, especially since most futures and I/O types can use the non-zero optimization.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 10, 2023

Right, but given PinnedDrop (good thing I was wrong about pin-project-lite not having it!) gives us Pin<&mut Self>, Option::take does not work and we'll have to resort to unsafe either way (I think). It also introduces pointless branches for every use of the inner field.

I'm working on a PR right now and it's quite gnarly (which might be due to ManuallyDrop to some extent, but I think it's mostly just due to pinning). I think it will be easier to discuss the solution in the context of an actual diff.

@notgull
Copy link
Member

notgull commented Oct 10, 2023

The value of a pinned type can be overwritten with set. So if you set the type to None it will work.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 10, 2023

Oohh, clever! I was aware of that method, but hadn't connected the dots. That should indeed make an unsafe-free solution possible. (together with Option::as_pin_mut which I found is also a thing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants