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

Tracking issue for Pin APIs (RFC 2349) #49150

Closed
withoutboats opened this issue Mar 18, 2018 · 211 comments
Closed

Tracking issue for Pin APIs (RFC 2349) #49150

withoutboats opened this issue Mar 18, 2018 · 211 comments

Comments

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Mar 18, 2018

Tracking issue for rust-lang/rfcs#2349

Blocking stabilization:

  • Implementation (PR #49058)
  • Documentation

Unresolved questions:

  • Should we provide stronger guarantees around leaking !Unpin data?

Edit: Summary comment: #49150 (comment) (in the hidden-by-default part)

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Mar 20, 2018

I now notice that stack pinning is not part of the RFC, @withoutboats are you planning on releasing a crate for this, or should I just copy the example code into my crate that needs it?

@withoutboats
Copy link
Contributor Author

@withoutboats withoutboats commented Mar 21, 2018

@Nemo157 You should copy it and report your experience!

The unresolved question about leaking Unpin data relates to this. That API is unsound if we say you cannot overwrite Unpin data in a Pin unless the destructor runs, as @cramertj requested. There are other, less ergonomic, stack pinning APIs that do work for this. Its unclear what the right choice here is - is the ergonomic stack pinning API more useful or is the extra guarantee about leaking more useful?

One thing I'll note is that the stack pinning was not sufficient for things like Future::poll inside the await! macro, because it didn't allow us to poll in a loop. I'd be interested if you run into those issues, and how/if you solve them if you do.

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Mar 21, 2018

My current usage is pretty trivial, a single-threaded executor running a single StableFuture without spawning support. Switching to an API like @cramertj suggests would work fine with this. I have wondered how to extend this to allow spawning multiple StableFutures, but at least with my current project that's not necessary.

@valff
Copy link
Contributor

@valff valff commented Mar 29, 2018

Just tried to experiment with the API. Looks like the following (suggested by RFC) definition of Future is no longer object-safe?

trait Future {
    type Item;
    type Error;

    fn poll(self: Pin<Self>, cx: &mut task::Context) -> Poll<Self::Item, Self::Error>;
}
@valff
Copy link
Contributor

@valff valff commented Mar 29, 2018

Nevermind. Found a note about a plan to make arbitrary_self_types object-safe.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 29, 2018

@withoutboats

One thing I'll note is that the stack pinning was not sufficient for things like Future::poll inside the await! macro, because it didn't allow us to poll in a loop.

Could you elaborate on that?

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 29, 2018

@RalfJung You need Pin to support reborrows as Pin, which it currently does not.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 30, 2018

@cramertj That sounds like a restriction of the Pin API, not of the stack pinning API?

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 30, 2018

@RalfJung Yes, that's correct. However, PinBox can be reborrowed as Pin, while the stack-pinned type cannot (one borrow as Pin creates a borrow for the entire lifetime of the stack type).

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 30, 2018

Given a Pin, I can borrow it as &mut Pin and then use Pin::borrow -- that's a form of reborrowing. I take it that's not the kind of reborowing you are talking about?

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 30, 2018

@RalfJung No-- methods like Future::poll were planned to take self: Pin<Self>, rather than self: &mut Pin<Self> (which isn't a valid self type since it isn't Deref<item = Self> -- it's Deref<Item = Pin<Self>>).

@withoutboats
Copy link
Contributor Author

@withoutboats withoutboats commented Mar 30, 2018

It might be the case that we could get this to work with Pin::borrow actually. I'm not sure.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 30, 2018

@cramertj I did not suggest to call poll on x: &mut Pin<Self>; I thought of x.borrow().poll().

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 30, 2018

@RalfJung Oh, I see. Yes, using a method to manually reborrow could work.

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Mar 30, 2018

I'll try and remember to post an example of some of the stuff I'm doing with Pins next week, as far as I can tell reborrowing works perfectly. I have a pinned version of the futures::io::AsyncRead trait along with working adaptors like fn read_exact<'a, 'b, R: PinRead + 'a>(read: Pin<'a, R>, buf: &'b [u8]) -> impl StableFuture + 'a + 'b and I'm able to work this into a relatively complex StableFuture that's just stack pinned at the top level.

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Apr 3, 2018

Here's the full example of what I'm using for reading:

pub trait Read {
    type Error;

    fn poll_read(
        self: Pin<Self>,
        cx: &mut task::Context,
        buf: &mut [u8],
    ) -> Poll<usize, Self::Error>;
}

pub fn read_exact<'a, 'b: 'a, R: Read + 'a>(
    mut this: Pin<'a, R>,
    buf: &'b mut [u8],
) -> impl StableFuture<Item = (), Error = Error<R::Error>>
         + Captures<'a>
         + Captures<'b> {
    async_block_pinned! {
        let mut position = 0;
        while position < buf.len() {
            let amount = await!(poll_fn(|cx| {
                Pin::borrow(&mut this).poll_read(cx, &mut buf[position..])
            }))?;
            position += amount;
            if amount == 0 {
                Err(Error::UnexpectedEof)?;
            }
        }
        Ok(())
    }
}

This is slightly annoying as you have to pass instances through everywhere as Pin and use Pin::borrow whenever you call functions on them.

#[async]
fn foo<'a, R>(source: Pin<'a, R>) -> Result<!, Error> where R: Read + 'a {
    loop {
        let mut buffer = [0; 8];
        await!(read_exact(Pin::borrow(&mut source), &mut buffer[..]));
        // do something with buffer
    }
}
@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Apr 3, 2018

I just had a thought that I could impl<'a, R> Read for Pin<'a R> where R: Read + 'a to workaround having to pass values as Pin<'a, R> everywhere, instead I could use fn foo<R>(source: R) where R: Read + Unpin. Unfortunately that fails because Pin<'a, R>: !Unpin, I think it's safe to add an unsafe impl<'a, T> Unpin for Pin<'a, T> {} since the pin itself is just a reference and the data behind it is still pinned.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 4, 2018

Concern: It seems likely that we want most types in libstd to implement Unpin unconditionally, even if their type parameters are not Pin. Examples are Vec, VecDeque, Box, Cell, RefCell, Mutex, RwLock, Rc, Arc. I expect most crates will not think about pinning at all, and hence only have their types be Unpin if all their fields are Unpin. That's a sound choice, but it leads to unnecessarily weak interfaces.

Will this solve itself if we make sure to implement Unpin for all libstd pointer types (maybe even including raw pointers) and UnsafeCell? Is that something we will want to do?

@withoutboats
Copy link
Contributor Author

@withoutboats withoutboats commented Apr 4, 2018

Will this solve itself if we make sure to implement Unpin for all libstd pointer types (maybe even including raw pointers) and UnsafeCell? Is that something we will want to do?

Yes, it seems like the same situation as Send to me.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 6, 2018

A new question just occurred to me: When are Pin and PinBox Send? Right now, the auto trait mechanism makes them Send whenever T is Send. There is no a priori reason to do that; just like types in the shared typestate have their own marker trait for sendability (called Sync), we could make a marker trait saying when Pin<T> is Send, e.g. PinSend. In principle, it is possible to write types that are Send but not PinSend and vice versa.

@withoutboats
Copy link
Contributor Author

@withoutboats withoutboats commented Apr 6, 2018

@RalfJung Pin is Send when &mut T is Send. PinBox is Send when Box<T> is Send. I see no reason for them to be different.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 6, 2018

Well, just like some types are Send but not Sync, you could have a type relying on "Once this method is called with Pin<Self>, I can rely on never being moved to another thread". For example, this could give rise to futures that can be sent around before being started for the first time, but then have to remain in one thread (much like they can be moved around before being started, but then have to remain pinned). I'm not sure if I can come up with a convincing example, maybe something about a future that uses thread-local storage?

bors added a commit that referenced this issue Sep 18, 2018
Update to a new pinning API.

~~Blocked on #53843 because of method resolution problems with new pin type.~~

@r? @cramertj

cc @RalfJung @pythonesque anyone interested in #49150
bors added a commit that referenced this issue Sep 19, 2018
Update to a new pinning API.

~~Blocked on #53843 because of method resolution problems with new pin type.~~

@r? @cramertj

cc @RalfJung @pythonesque anyone interested in #49150
@Pauan
Copy link

@Pauan Pauan commented Oct 22, 2018

I've had to learn the Pin API due to my work with Futures, and I have a question.

  • Box unconditionally implements Unpin.

  • Box unconditionally implements DerefMut.

  • The Pin::get_mut method always works on &mut Box<T> (because Box unconditionally implements Unpin).

Taken together, that allows for moving a pinned value with entirely safe code.

Is this intended? What is the rationale for why this is safe to do?

@tikue
Copy link
Contributor

@tikue tikue commented Oct 22, 2018

It looks like you have Pin<&mut Box<...>>, which pins Box, not the stuff inside Box. It's always safe to move a Box around, because Box never stores references to its location on the stack.

What you likely want is Pin<Box<...>>. Playground link.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Oct 22, 2018

EDIT: no longer accurate

@Pauan Just because a Box is pinned does not mean the thing inside it is pinned. In other words, you cannot get a Pin<Foo> from a Pin<Box<Foo>>, and the latter does not provide the same guarantees as the former. Any code depending on this would be incorrect.

The thing you are looking for is probably PinBox, which disallows the behavior you mentioned, and allows you to get a PinMut<Foo>.

@tikue It’s still possible to move out of a Pin<Box<...>>, which I think is what they were trying to avoid.

@tikue
Copy link
Contributor

@tikue tikue commented Oct 22, 2018

@tmandry Correct me if I'm wrong, but Pin<Box<...>> pins the thing inside the Box, not the Box itself. In @Pauan's original example, they had a Pin<&mut Box<...>>, which only pinned the Box. See my playground link showing how Pin<Box<...>> prevents getting a mutable reference to the thing in the box.

Note that PinBox was recently removed, and Pin<Box<T>> now has the same semantics as PinBox.

@Pauan
Copy link

@Pauan Pauan commented Oct 22, 2018

@tmandry PinBox<T> has been removed and replaced with Pin<Box<T>> on Nightly (the doc link you gave is for Stable). Here is the correct Nightly link.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Oct 22, 2018

Oh, the rules must have changed since I last used these. Sorry for the confusion.

@Pauan
Copy link

@Pauan Pauan commented Oct 22, 2018

@tmandry Yes, the changes were very recent. Since things are still in flux, it's hard to keep up with all the changes.

@withoutboats
Copy link
Contributor Author

@withoutboats withoutboats commented Oct 22, 2018

The comment by @tikue is correct. You need to remember that pins only pin one level of indirection down.

@Pauan
Copy link

@Pauan Pauan commented Oct 23, 2018

@tikue @tmandry @withoutboats Thanks for the answers! It was very helpful.

@crlf0710
Copy link
Contributor

@crlf0710 crlf0710 commented Oct 28, 2018

So what are the states of the two concerns now? (api-refactor & get_mut_unchecked_mut_mut) As someone who's eagerly waiting for the async/await series feature, I wonder which rustc version will the Pin APIs target? Is there an estimation?

@withoutboats withoutboats mentioned this issue Nov 7, 2018
0 of 5 tasks complete
@aturon
Copy link
Member

@aturon aturon commented Nov 9, 2018

@crlf0710 see the stabilization proposal.

bors added a commit that referenced this issue Dec 12, 2018
Expand std::pin module docs and rename std::pin::Pinned to PhantomPinned

cc #49150, #55766

r? @withoutboats
@Centril
Copy link
Contributor

@Centril Centril commented Dec 27, 2018

@withoutboats Seems done? Shall we close?

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Feb 11, 2019

Ok I apologise if this is not the place to post this, but I have been thinking about the Drop + !Unpin issue, and have come out with the following idea:

  1. Ideally, if Drop::drop was fn(self: Pin<&mut Self>) there would be no issue. Let's call such a Drop PinDrop. We cannot just replace Drop by PinDrop because of retrocompatibility issues.

  2. since the only issue with Drop::drop(&mut self) is for the Drop + !Unpin case, we could derive a default impl of PinDrop for Drop + Unpin (since then Pin<&mut T> : DerefMut<Target = T>) and make PinDrop be the trait automagically used by rustc (thanks to Pin::new_unchecked(&mut self), since drop is the only case of stack pinning when we think about it).

Here is a sketchy PoC of the idea: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=9aae40afe732babeafef9dab3d7525a8

Anyways, imho this should remain in beta and not go stable yet, even if it has to be an exception. If there is a time where Drop behavior can depend on Unpin without breaking compat then that time is now.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 11, 2019

@danielhenrymantilla I don't see how that solves the problem of compatibility with existing generic drop impls, like impl<T> Drop for Vec<T>.

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Feb 11, 2019

You're right it requires another thing:
an implicit T: Unpin bound on all generics, with an opt-out using ?Unpin in the same manner as Sized

That should make it become

impl<T> Drop for Vec<T>
where
  T : Unpin, // implicit, which implies that Vec<T> : Unpin which "upgrades" `Drop` into `PinDrop`
@RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 11, 2019

an implicit T: Unpin bound on all generics, with an opt-out using ?Unpin in the same manner as Sized

This has a huge effect on the overall API design, and was discussed extensively as part of the ?Move proposals. For example, it would mean that many, many existing libraries would need updating to work with pinning. The conclusion was that going with a library-only solution such as what we got now is better because it requires none of this.

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Feb 11, 2019

Yep, huge cost short term since all existing libraries would need to update to be !Unpin compatible, but in the long run we would end up with a "safer" Drop. It did not seem that bad at first, since at least we are not breaking anything.

But it is a fair concern (I did not know that it had been raised previously; thank you for pointing it out, @RalfJung ), and I guess that the short term practical drawbacks do out-weight the little safety gain of a drop(Pin<&mut Self>).

@shanemikel
Copy link

@shanemikel shanemikel commented Apr 23, 2019

Has there been any discussion on Hash implementation for Pin types hashing on addresses?

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Apr 23, 2019

Pin should probably have an implementation of Hash that simply delegates to the contained pointer, there's no precedence for hashing based on addresses (and I don't see any reason that pinning a value should change how it gets hashed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.