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

Add Send trait bounds to the Futures #185

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Add Send trait bounds to the Futures #185

merged 2 commits into from
Oct 30, 2018

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Sep 5, 2018

Tokio's executor trait requires Send to be implemented on the future to be spawned.

@vi
Copy link
Member

vi commented Sep 5, 2018

Does it limit ability to use rust-websocket with non-sendable underlying connection while single-threaded?

@tomaka
Copy link
Contributor Author

tomaka commented Sep 5, 2018

@vi It does indeed. However I have yet to find any connection in the wild that isn't Send. Plus, as it has to be conservative, the spawn method of tokio requires Send (https://docs.rs/tokio/0.1.8/tokio/executor/fn.spawn.html) even when the executor wouldn't in fact require Send.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 5, 2018

The ideal solution is not to use a Box, but that's a rather big change.

@vi
Copy link
Member

vi commented Sep 5, 2018

However I have yet to find any connection in the wild that isn't Send

Non-Send world is just one Rc<RefCell> away. If a program is designed to be single-threaded, there may be no Mutex'es (with their poison and overhead, etc.), but Rc instead.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 5, 2018

On the other hand, right now it's impossible to use this library both in a multithreaded context and in a single-threaded context with the latest version of tokio.

@vi
Copy link
Member

vi commented Sep 5, 2018

Will it stay that way with Tokio reform done?

@vi
Copy link
Member

vi commented Sep 5, 2018

One of the hacks that can be done is to let users to choose "Provide Send, but requre Send" or "Not provide Send, not require Send" as Cargo feature (like #[cfg(feature="send")]). The feature may be default-on.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 5, 2018

Using a feature is bad idea, because if a user pulls the websocket library once without the feature and once with the feature, then the feature will be enabled even on the side where it wasn't.

In other words, code that compiles with a feature disabled should always keep compiling with that feature enabled.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 5, 2018

Will it stay that way with Tokio reform done?

This change is "post tokio reform". You can consider it part of the reform.

@vi
Copy link
Member

vi commented Sep 5, 2018

Shall Mutex-denial users use

/// Not actually safe, but we are not using threads anyway and library requires Send unconditionally
unsafe impl Send for SomeWrapper{}

hack? (like I use in my project)

It starts feeling that although theoretically one can use do Futures without Send with current_thread runtime, in reality it not ecosystem-supported (like no_std - you can do no_std, but most dependencies bring in std)...

@vi vi requested a review from illegalprime September 6, 2018 14:33
@dicej
Copy link

dicej commented Oct 29, 2018

@vi You might want to consider https://crates.io/crates/send_wrapper as a safe alternative to unsafe impl Send.

@vi
Copy link
Member

vi commented Oct 29, 2018

Primary authors and maintainers (@cyderize, @illegalprime) inactivity watchdog is expired, so I'm going to start releasing rust-websocket versions with some pull requests merged myself, soon-ish.

Does this pull request grant a minor version (0.20.4) or better to be safe and bump it to 0.21.0?

@vi vi merged commit 28ea5eb into websockets-rs:master Oct 30, 2018
@vi
Copy link
Member

vi commented Oct 30, 2018

Published websocket v0.20.4 with this change.

@tomaka tomaka deleted the send branch October 30, 2018 17:59
@tomaka
Copy link
Contributor Author

tomaka commented Oct 30, 2018

That should be 0.21 strictly speaking, as there is a new trait bound requirement.

@vi
Copy link
Member

vi commented Oct 30, 2018

Already released.

I plan to publish 0.21.0 with #186, and maybe 0.22.0 from @enzious.

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.

3 participants