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

We should clarify how e-h-async traits should behave in the face of cancellation #632

Open
jamesmunns opened this issue Sep 24, 2024 · 3 comments

Comments

@jamesmunns
Copy link
Member

Should we specify behaviors for drop/cancellation, or even mem::forget for embedded-hal-async methods? This impacts all async methods, as well as things like SpiBus.

See https://libera.irclog.whitequark.org/rust-embedded/2024-09-24#1727191016-1727191658; for brief chat conversation history.

Originally posted by @jamesmunns in rust-embedded/wg#796 (comment)

@MabezDev
Copy link
Member

We should definitely mention that mem::forget'ing a transaction may have consequences, but IMO enforcing mem::forget safety in the eh contract is a step too far.

I understand the obvious reasons for wanting it, but how often is it an issue in reality? Remember that firstly, it's something the user has to do themselves (and how often does one reach forget in everyday Rust code anyway? ) but also there are clippy lints to detect forgeting a struct with a Drop impl, which at least in esp-hal we recommend enabling.

The above is my opinion, and I understand that many people will value complete safety over everything; I think that's a good thing. I also think it's a choice that can be made without being written into the eh contract.

@jannic
Copy link
Member

jannic commented Sep 25, 2024

mem::forget is not the only way to forget a value - that is, to free an allocation without calling Drop. Eg. you can do similar things with MaybeUninit::new, which could more likely happen by accident.

But I agree that we should acknowledge that reasonable implementations of the e-h-async traits may not follow the soundness rules of rust to the letter, and may cause UB if Drop is somehow skipped. Even without judging if this is a good choice, it's just a matter of fact that embassy made that choice, and embassy is a very important part of the embedded rust ecosystem: It's both extremely useful and popular. So I guess driver authors don't want their drivers to break when used with embassy.

However, I'd prefer to find some wording that doesn't encourage that choice unconditionally. Even though there are good reasons to deviate from the official Rust rules, it's still a tradeoff that should only be made after carefully considering alternatives. Ideally, users and implementors of the traits should be conservative: Don't rely on Drop for safety if possible, but also don't forget objects without calling Drop. And wherever they deviate from these rules, they should clearly document it.

@kevinmehall
Copy link

I think it's actually quite hard in practice for no_std code to violate the stricter version of the pin-drop guarantees that Embassy assumes. Neither #[task] nor pin!() allow leaking a future once it's been pinned.

The main way with std to pin something and later leak it involves Box. The "must remain, valid, at that same address in memory, until its drop handler is called" part of the pin-drop requirement was basically written to allow a Box containing a pinned value to leak, but taking advantage of that requires something that works like a heap, leaking memory that it will never touch again. If the memory is on the stack or in a static that will ever be re-used, you're obligated to call Drop.

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

No branches or pull requests

4 participants