Arc::ptr_eq equivalent for Stealer/Worker

[james7132]

It's not currently possible to tell whether or not a Stealer or Worker are pointing to the same underlying queue. Could we get a public facing API for this?

[sbarral]

Hi, thanks for filing the issue and for the PR.

I would be OK if this can be achieved with a somewhat less ad-hoc approach. I have outlined a proof of concept, please let me know if that would work for you. You could then achieve what you wanted as follow:

if my_worker.stealer_ref() == &my_stealer { ... }

If this approach suits you, then feel free to modify your PR along those lines (or I can add tests similar to yours and make a PR out of the PoC, just let me know).

[james7132]

The only objection I have to the POC is that the transmute is shady even with the repr(transparent) and assumes that references between types inherently have a fixed ABI. It's not a strict lifetime transmute and really hits the edge cases of what's sound, and I would generally appreciate a solution that minimized exposure to unsafe.

The PartialEq/Eq on stealers is nice and works for me.

[sbarral]

My understanding of repr(transparent) is that it is precisely meant to warrant ABI equivalence. It is true that the ABI is not "fixed" in the sense that it can change between rust releases (unlike say repr(C)), but I don't see how that could result in UB.

The Rust RFC book also says [...] "without actually being affected by any ABI semantics", which to me suggests that this falls squarely within the safe uses of transmute. I also found a few threads like this one that appear to confirm this view.

Now to be clear, I agree that this should not be used and will definitely reconsider if you can point me to discussions that indeed suggest this is an edge case WRT soundness.

In the worse case there is the easy way out: storing a Stealer within the Worker instead of storing directly the underlying queue. Ugly but 100% safe ;-)

The reason I prefer PartialEq/Eq + stealer_ref rather than is_same_as is that they are orthogonal and both useful in their own right, whereas is_same_as is probably a bit niche (crossbeam's deque does not seem to have anything like this despite being way older than St3).

[james7132]

In the worse case there is the easy way out: storing a Stealer within the Worker instead of storing directly the underlying queue. Ugly but 100% safe ;-)

I'd strongly prefer this over the transmute.

The reason I prefer PartialEq/Eq + stealer_ref rather than is_same_as is that they are orthogonal and both useful in their own right, whereas is_same_as is probably a bit niche (crossbeam's deque does not seem to have anything like this despite being way older than St3).

Fair. My only other major concern is that it returns a reference of a Send/Sync type into the internals of a !Send/!Sync type. While not inherently unsound, it has me suspicious about it's use, even if the core Arc points to the same piece of memory.

[sbarral]

Fair. My only other major concern is that it returns a reference of a Send/Sync type into the internals of a !Send/!Sync type. While not inherently unsound, it has me suspicious about it's use, even if the core Arc points to the same piece of memory.

This is in a way why I would argue that storing a Stealer inside the Worker is a plain misuse of the wrapped stealer. The methods of the worker are sound only if Worker is !Sync, but if Worker became a wrapper for a Stealer it would be actually necessary to negate the Sync by introducing an appropriate PhantomData.

On the other hand, there is IMO absolutely nothing wrong in returning a reference to a Stealer type (abstracting over how it is internally implemented). In fact there would be nothing wrong in implementing the methods of Stealer on a Worker: the only reason this is not done is that it would be inelegant and confusing, so stealer_ref provides a zero-cost escape hatch for the cases where it may needed.

I guess my point is: using unsafe is never a light decision, but storing a Stealer would only serve to give ourselves a false sense of security and pretend that the code is safe when it actually isn't: without a PhantomData to make it !Sync it would be plain UB even though there is technically no new occurrence of the dreaded unsafe keyword. I am of the firm belief that authors should not hide a potential unsafe and that only counting occurrences of unsafe is as little relevant as LOC metrics (unless one has a fully safe codebase of course). In fact I will frequently split an unsafe section into several smaller unsafe sections so they can be justified individually and can be more easily challenged in reviews -- and our conversation shows that this latter goal is achieved ;-)

Your reluctance is noted and understood, and as said above I am ready to reconsider if some gotchas are uncovered regarding transmute on repr(transparent) types, but otherwise I would rather live with an unsafe that can be easily reviewed than with a solution that looks safe but would actually be UB without due precautions. But I totally get that you would prefer not to undersign on this so I can make tomorrow a PR based on the PoC, in which case I would be grateful if you would grant me approval to adapt your tests.

[sbarral]

I would be grateful if you would grant me approval to adapt your tests.

@james7132 I did not get your response on that, should I go on with integrating your tests and merging the PoC? If you are not OK with the proposed course of action then no problem, I would then leave it up to you to close the issue or keep it open (maybe a more consensual way to achieve can still be found, though I ran out of ideas for now).

[james7132]

Oh completely missed that! Sorry, please do go ahead.