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

Unwind safety #28

Closed
rainhead opened this issue Jan 15, 2020 · 4 comments
Closed

Unwind safety #28

rainhead opened this issue Jan 15, 2020 · 4 comments

Comments

@rainhead
Copy link

Hi there, and thanks for your work on this library.

I don't feel very confident on how unwind safety works, but I noticed that
std::cell::UnsafeCell<futures_intrusive::channel::mpmc::ChannelState<ControlMessage<A>, futures_intrusive::buffer::ring_buffer::if_std::HeapRingBuf<ControlMessage<A>>>> may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary

Is this in fact true, or are we missing a marker trait?

@Matthias247
Copy link
Owner

I can't really tell you what the message you are referering means.

Regarding unwind safety: The library assumes all of it's internals will never panic!. Therfore special unwind handling is not required. Since no unknown user-code is run as part of the library this is a reasonable assumption.

@rainhead
Copy link
Author

My understanding is that UnsafeCell is !RefUnwindSafe by default, and you must excplicitly mark your wrapper type, here GenericChannelSharedState, as RefUnwindSafe. See also rust-lang/rust#54768.

Would you accept a PR for marking your shared state structs unwind safe?

@Matthias247
Copy link
Owner

I'm afraid I can't really answer the question right now, since I'm not comfortable enough with the meaning of UnwindSafe and RefUnwindSafe, and what they cover. I would rather not add markers that e.g. claim something is safe that later on doesn't hold true.

If they are talking about unwindiding e.g. with a Channel on the stack, but due to a panic outside of futures-intrusive - that is ok and totally fine to do. A panic inside futures-intrusive however is not expected and all bets are off.

Can you tell me your use-case for this?

The UnsafeCell is in the RingBuffer implementations, so if something needs to be annotated it would probably be that one and not the SharedState things.

@Matthias247
Copy link
Owner

I'm closing this due to no feedback

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

2 participants