Don't require Unpin when pin projecting. #89
Conversation
There are two scenarios here. Either you want users to be able to pass in streams that are `!Unpin`, in which case pin_project and the compiler have your back and make sure this works, or... You are going to require Unpin on it in which case you can remove the pin projection. Both together don't make sense. Either one works here, but by removing the Unpin the API is less restrictive, which I would favor, even if I don't immediately see what concrete `!Unpin` type someone might want to call this with, but sooner or later someone probably will, so why disable if it can work. This leaves another problem though, there are many other places in the lib where `Unpin` is required, notably on the wrappers around `Stream`, so it's not said that with this change users can actually use any `!Unpin` streams. There is one other use of pin-project on `MidHandshake`. As far as I can tell the effect of this one is that `Role::InternalStream` can now be `!Unpin` in this specific file, but is it even possible to use the lib with an `!Unpin` type for this? It looks like it might be good to rethink the choices about pin for the entire lib and make something consistent. Maybe aim for the largest API compatibility by trying to never require `Unpin` and seeing if pin-project makes that possible. In any case, the Unpin in `Stream` has no good reason for being there unless you would like to [get rid of pin-project](18cc347). As an added bonus, if you pin project, there is no need for re-pinning in the match.
|
@sdroege if I'm not mistaken, you added pin projection here. I did spot the code first in async-tungstenite, so I think that has the same issues. If there is confusion about the pin API, don't hesitate to ask, but as long as mr unsafe is not involved, you can't kill puppies by accident. |
Yeah, previously this was using unsafe code instead. I agree that getting rid of the |
|
Ok, I'll see if I can make time to look through the rest of the lib. |
|
I started to look through both libs to see what it entails to work on underlying streams that are However, it's not ergonomic, it's a breaking change, and for what? Has anyone got a usecase for an As a conclusion, I would lean towards keeping the The choice is reversible with a breaking change, so rather than making a breaking change now and making the API less ergonomic, it might as well be done if and when a use case presents itself. |
| Stream::Plain(ref mut s) => Pin::new(s).poll_read(cx, buf), | ||
| Stream::Tls(ref mut s) => Pin::new(s).poll_read(cx, buf), | ||
| Stream::Plain(s) => s.poll_read(cx, buf), | ||
| Stream::Tls(s) => s.poll_read(cx, buf), |
|
Superseded by #194 |
There are two scenarios here. Either you want users to be able to pass in
streams that are
!Unpin, in which case pin_project and the compilerhave your back and make sure this works, or...
You are going to require Unpin on it in which case you can remove the pin
projection. -> See #90
Both together don't make sense. Either one works here, but by removing the
Unpin the API is less restrictive, which I would favor, even if I don't
immediately see what concrete
!Unpintype someone might want to call this with,but sooner or later someone probably will, so why disable if it can work.
This leaves another problem though, there are many other places in the lib
where
Unpinis required, notably on the wrappers aroundStream, so it'snot said that with this change users can actually use any
!Unpinstreams.There is one other use of pin-project on
MidHandshake. As far as I cantell the effect of this one is that
Role::InternalStreamcan now be!Unpinin this specific file, but is it even possible to use the lib with an
!Unpintype for this?
It looks like it might be good to rethink the choices about pin for the
entire lib and make something consistent. Maybe aim for the largest API
compatibility by trying to never require
Unpinand seeing if pin-projectmakes that possible.
In any case, the Unpin in
Streamhas no good reason for being there unlessyou would like to get rid of pin-project.
As an added bonus, if you pin project, there is no need for re-pinning in the match.