Repository navigation
Tracking issue for Weak::into_raw/from_raw & similar #60728
Description
Changing this to a tracking issue for the feature:
- Pull request Weak::into_raw #60766
- ❓ What to do about dangling Weak (created by
Weak::new()) andT: ?Sized? These seem to be incompatible. - Stabilization
The original proposal
Hello
The Arc has the into_raw and from_raw methods. I think it would be technically possible to have the same on Weak. Obviously, as Weak is non-owning, it would be up to the caller to make sure the pointer is not dangling when used.
Would adding these (and maybe as_raw too ‒ one can get a reference out of Arc, but not from Weak) require an RFC, because the handling of these methods might be even a bit more delicate than the ones on Arc, or is this considered small enough for just a pull request & later stabilization?
Motivation
I've written the arc-swap crate that allows to keep an Arc around but make it point to some other object atomically. It uses the mentioned methods. It would make sense to have weak version of this atomic storage too.
Additionally, the as_raw would make it possible to compare if eg an Arc and Weak point to the same object (which would also come handy in some alternative of the Cache that doesn't hold the object alive needlessly).
Activity
Centril commentedon May 11, 2019
A PR is sufficient; the motivation can be justified in there.
[-]Weak::into_raw/from_raw & similar[/-][+]Tracking issue for Weak::into_raw/from_raw & similar[/+]Rollup merge of rust-lang#60766 - vorner:weak-into-raw, r=sfackler
Rollup merge of rust-lang#60766 - vorner:weak-into-raw, r=sfackler
cramertj commentedon Oct 3, 2019
@rust-lang/libs is there any opposition to stabilizing at least
.as_raw()? It's hard to imagine wanting that function looking any different than it does today, and it's pretty useful (I don't know another way to deduplicate weak pointers to the same object).vorner commentedon Oct 4, 2019
When you bring it up, what needs to be done for stabilizing it all? The API follows the one on
Arc, so I don't think any of this could look differently.There's the problem with unsized
Ts ‒ currently, they are simply not supported (because of the interaction with dangling & null pointers). But relaxing the restriction later would be backwards compatible change, so I don't think a decision what exactly to do about that has to block stabilization.CAD97 commentedon Nov 21, 2019
Relaxing the restriction of
Tto?SizedI don't think is compatible with the current safety requirements. On top of that, I think that as is currently written, it's self-conflicting.The safety requirement requires that the pointer point to a valid
ArcInner/RcBox(side note: probably should make those naming consistent). But then it also says that it acceptsnull? The reason for this is thatas_raw/into_rawreturnnullwhenthe strong count is 0when the weak is fake (self.inner()returnsNone).But on top of that, this is wrong (I just realized writing this up)! Becausefrom_raw(into_raw(_))on a danglingWeakturns it into the singleton danglingWeak, the weak count of the original will never be decreased! The correct behavior is forWeak::into_rawto return the dangling pointer such thatWeak::from_rawcan recover the original pointer to allocation and decrease the weak count. This then is fully compatible withT: ?Sizedas it's doing the exact same as the non-weak version.As for
as_raw... it's "fine" for it to returnnullas a helpful hint (as dereferencingnullis a faster error), but I think it's more important foras_raw(_); forget(_)to be equivalent tointo_raw(_).vorner commentedon Nov 24, 2019
It's been a while since I wrote it, so I went back to have a look. I think it is slightly different than how you describe it. If I read through it correctly, there are three kinds of weak pointer values:
strong_count() > 0.strong_count() = 0. They've run the destructor on theT, but the RcBox is still there.Weak::new(). They don't have any RcBox allocated for them and their pointer is set to a sentinel value (usize::MAXcasted to pointer).The
into_raw()/from_raw()returnnull()only for the last case. There's no weak count to speak of in that case, so nothing is forgotten to be decreased.The code needs to special-case the sentinel value somehow anyway, it can't just add the offset to it, because it would overflow and that is AFAIK forbidden thing to do with pointers. It could also return the sentinel value (
usize::MAX), but null seems to be as good sentinel value as that other one, with the difference users recognize it better. It also has proper alignment for the type, which other code might rely on (by putting flags into the low bits). The downside is that pointers to unsized types don't havenull()and if I remember correctly, this was the reason why they are not currently supported.I can try changing the sentinel value and see if I can add support for unsized types instead.
As for the documentation, if I just remove all notions of null and keep it to the fact the pointer must come from previous
Weakand not specify what value is returned in case it is fromWeak::new(), would it be better?CAD97 commentedon Nov 24, 2019
Ah, I admit, I read the code wrong.
Ok, I agree returning
nullfor "fake" weak pointers is probably the correct sentinel. (The use ofusize::MAXas a sentinel for the inner pointer only works because theRcBox/ArcInnerhasalign(usize) > 1, but theTdoesn't have that guarantee, sousize::MAXis a theoretically valid pointer. I think aTpointer within anRcBox/ArcInneris guaranteed to have at leastalign(inner)because of the alignment of the container, but it'd be better to not rely on that.)But the nonexistence of
ptr::nullfor?Sizedtypes isn't really an issue. The reason it doesn't exist is the inability for it to conjure pointer metadata out of thin air. We have that!I don't know the exact way std would accomplish this, but "all" that needs to be done is to pull the metadata off of the inner pointer, and put it onto a null pointer. (And then do the same thing in
from_raw.)For docs, I'd just specify that the pointer given to
Weak::from_rawmust have come fromWeak::as_raworWeak::into_raw.vorner commentedon Nov 24, 2019
I've tried updating the docs, I hope it's for the better.
It's not the problem with null pointers per se, because
Weak::newexists forSizedtypes only too. It's simply illegal to putnullpointer intointo_rawfor unsized type for that reason. However the sized implementation needs to check fornullequality. It would be fine if this turned out to befalseevery time for unsized types, but as thenull()doesn't even exist, it doesn't compile :-(. I think specialization would help here but AFAIK it's not ready to be used yet. Or is it?But, after clarifying the documentation, is there anything that would prevent adding that specialization/relaxing the bounds in the future in backwards compatible manner?
27 remaining items
vorner commentedon May 15, 2020
@Kixunil Can you explain what you mean? It was renamed to
as_ptrto hint that it should not be interchangeable with theinto_raw. The documentation no longer suggests it should be possible to useas_ptrto re-create it withfrom_rawand then forget it. So I think your concern was already resolved?rfcbot commentedon May 15, 2020
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
Kixunil commentedon May 16, 2020
@vorner I mean not having it at all. :)
into_rawis much more useful in my experience. If not for @CAD97 use case, I would believeas_ptrto be completely useless.vorner commentedon May 16, 2020
There are other use cases. Like, hashing it, equivalence checking, etc.
Kixunil commentedon May 16, 2020
@vorner aren't those cases better served by using a reference and converting it? Seems more obvious what it does.
vorner commentedon May 16, 2020
What reference do you have in mind?
&Weak<T>is not useful for these cases (where you want to check if eg. an atomic pointer still points to the same thing as the Weak you have cached or if you want to recreate a new one). You don't get&Tout of aWeakand have to upgrade it to theArc. But that is both more expensive and might returnNone‒ but you might want to know that theWeakyou have is still the one you should have even if it is no longer valid. Theas_ptron Arc is there mostly for symmetry.But I don't really understand your issue. What kind of misuse are you afraid of?
as_ptrdoesn't seem to allow more foot guns thaninto_raw‒ you still get a raw pointer out of that and can do whatever you like with it. You also need to useunsafeto do anything dangerous with it, which should be a lint enough to stop people from doing stupid things by accident.I assume that when the FPC is over, I can enjoy submitting a stabilization PR.
Kixunil commentedon May 17, 2020
Ah, yeah, I missed that. I had strong ones in mind mostly. Might be a good reason to have
ptr_eqandhash_ptrmethods instead, but pointer is definitely more flexible. But the docs should probably explain the pointer is useless for anything else than comparison.I was mostly wondering about
as_ptrbeing confused withinto_ptrin case of strong versions. A sad thing about these kinds of bugs is when combined with multithread environment they can be really hard to debug.(Personal experience: code sending a pointer from one thread to another waiting on select over a Unix pipe, the sender forgot to ref the pointer. Took years to find the cause of mysterious crashes.)
vorner commentedon May 17, 2020
That wouldn't allow customizing how you hash or against what you compare the pointer (I'd say
ptr_eqshould take anotherWeakas a parameter and in case of storing the pointer out-of-bound, you have*const T, maybe).But that is not true. The pointer is fully usable as long as you make sure by some other means that the lifetime of the object is still valid, same as with any other pointer.
Kixunil commentedon May 17, 2020
Yeah, it's possible to do it by other means, I just can't imagine a real-life situation in which one would have a
Weakand know that it's valid without also havingRc/Arcand using a reference to those instead.That all being said, it's not a huge deal, I guess. Mainly wanted this to be considered.
Stabilization of weak-into-raw
Rollup merge of rust-lang#72288 - vorner:stabilize-weak-into-raw, r=d…
Rollup merge of rust-lang#72288 - vorner:stabilize-weak-into-raw, r=d…