search

tokio-rs / slab

Slab allocator for Rust
MIT License
699 stars 84 forks source link

convert Entry to Result and functionalize code #47

Closed vadixidav closed 5 years ago

vadixidav commented 6 years ago

This primarily converts Entry to Result and then utilizes that fact to simplify the code.

I also added a large comment block in remove() explaining the significance of the state correction in remove() in regards to its relation to different types of stack unwinding unsafety and what conditions would need to exist to remove the state correction without compromising the current safety. This is necessary so in the future it is understood by maintainers why it has this behavior and which scenarios (such as panic = abort) it can be elided in.

vadixidav commented 5 years ago

Is this still desired? I can resolve the merge conflicts.

carllerche commented 5 years ago

Sorry for the delay. This got lost in my inbox :(

I see where you are coming from with this change. However, my gut is that this is not an idiomatic use case for Result. Result intends to represent success / error variants and is not a general purpose "either" structure.

I would be happy to keep just the comment though.

Thanks 👍

carllerche commented 5 years ago

I will close this PR now. If you have interest, please open a new PR w/ just the comment. I would add a clarifying note explaining that the comment pertains to re-assigning back to the slot before panicking.

tormol commented 5 years ago

I'm almost certain the comment is wrong.

Slab is not able to cause undefined behavior because it uses no unsafe code internally. If it could, that would be a soundness bug in the compiler, and none of the currently open ones appear relevant. The Unsafe book makes a nice distinction between minimal and maximal unwind / exception safety. While minimal exception safety is required to prevent UB, that is only an issue for unsafe code. Maximal exception safety is about preventing inconsistent (but safe) states from being observed, and this is what lock poisoning and catch_unwind() without AssertUnwindSafe provides. Restoring the entry in Slab.remove() provides maximal unwind safety, which is great when doing it is this simple, but it is not required for minimal unwind safety or to prevent UB.

panic = "abort" aborts the process, not the thread, which makes cross-thread synchronization not relevant. (unless one is sharing memory with another process, but I don't think one can use Slab for that.)

vadixidav commented 5 years ago

@tormol Got it. If I open another PR, I will explain that it is there to protect against maximal unwind safety. I might have used the term "undefined behavior" a little too loosely. The problem in this case is that it puts the Slab into an incorrect state, but that probably wont actually cause undefined behavior, which is what I erroneously called this.

tormol commented 5 years ago

I think what would happen is that it creates a loop in the vacant list, which will make a future insert (but not the next one!) panic because the entry it replaces was occupied. I can imagine that would be hard to debug.

I commented here because the comment that is currently in the code made me implement (maximal) unwind safety in the features I'm adding, but I felt it wasn't worth the code complexity there. Realizing how hard it could be to debug is actually making me reconsider that view though.