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

remove Unpin requirement for the inner stream #358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BiagioFesta
Copy link

Currently, WebSocketStream requires Unpin being implemented for the underlying I/O resource.

The aim of this PR is to remove this constrain.

Context

Any underlying I/O streams that work with timeouts logic won't implement Unpin.

For example: tokio-io-timeout::TimeoutStream it does not implement Unpin.

That's because any timer related future over tokio runtime is !Unpin.

Of course, an easy solution would be to allocate the underlying stream on the heap for pinning (e.g., Pin<Box<T>>). However, that means heap allocation and pointer indirection. Although those drawbacks are probably not really impactful, removing Unpin requirement will make this library more flexible.

On the other hand, removing Unpin does not come for free:

  1. Adding additional dependency: pin_project.
    • I think this is not a real concern. pin_project is very very widely spread. It is likely indirectly pulled in anyway.
  2. unsafe code.
    • This is real drawback of this PR. The tread-off here is: library more flexible but with unsafe blocks.

})
.map(|r| {
unsafe {
self.get_unchecked_mut().ready = true;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing SAFETY comment:

// SAFETY: not moving out

@Dushistov
Copy link

think this is not a real concern. pin_project is very very widely spread

tokio depend on pin-project-lite, not pin-project. futures-util also depends on pin-project-lite.

@BiagioFesta
Copy link
Author

think this is not a real concern. pin_project is very very widely spread

tokio depend on pin-project-lite, not pin-project. futures-util also depends on pin-project-lite.

I guess we can switch to pin-project-lite, still not the real concern of this change I guess

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

Successfully merging this pull request may close these issues.

2 participants