Don't project if you require Unpin. #90
Conversation
This doesn't make sense. For Unpin types, Pin does nothing, so Pin::new() is sufficient and there is no need to project. If it's projected, it's already pinned, so no need to call Pin::new(). Thus project and Pin::new together is always a code smell.
|
@sdroege what do you think of this solution? Would you like an equivalent PR on async-tungstenite? |
|
I think it makes sense, yes |
|
oh, and what I didn't think of before, users should still be able use |
There was a problem hiding this comment.
Thanks for tackling this issue, @najamelan . I did not think that deep about this issue previously. Well, for me it seems that the approach suggested here (#90) would indeed be a better solution for now and additionally we get rid of the pin-project dependency "for free".
@agalakhov what do you think? #89 vs #90 ?
|
Note that async-tungstenite, which is a fork with almost identical code chose this path. I think #89 only makes sense if a real path is demonstrated which allows using !Unpin streams, which I didn't find easily. |
@agalakhov , what's your opinion on that? |
|
I think #90. It looks like safer approach to me. |
|
Superseded by #194 |
This is the counter proposition of #89. Since it's rather convoluted to remove the
Unpinrequirement on the underlying stream users can wrap inWebSocketStreamand I don't know of any use cases for it, I propose to keep theUnpinand consequently remove pin-projection from tokio-tungstenite. This also sheds a dependency.The first commit is completely self contained. Nothing outside of this file will observe any difference. I think it's good to apply it no matter what except if choosing to remove Unpin bounds everywhere as discussed in #89.
The second is a bit more complicated. It's concerned with the handshake, which takes place before there even is a
WebSocketStream. There is a typeRole::InternalStream, which comes from tungstenite, which uses trait boundsRead + Writeand doesn't mentionUnpin. The current state allowed this type to be!Unpin, although I suspect that normally this is the same underlying connection that is used for wrapping inWebSocketStream, so there probably already wasn't any way for users to pass a!Unpintype in here.However, I haven't actually tested this assumption. If the assumption holds, I suppose this is not a breaking change. If it changes what people could pass in, it is a breaking change.