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

Conversion to tokio 0.2 #68

Merged
merged 7 commits into from Jan 7, 2020
Merged

Conversion to tokio 0.2 #68

merged 7 commits into from Jan 7, 2020

Conversation

@dbcfd
Copy link
Contributor

dbcfd commented Sep 9, 2019

Convert to tokio 0.2

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 9, 2019

currently hung up on tokio-dns, looks like the future it returns isn't correct, Box<Future> rather than impl Future or Pin<Box<Future>>

@najamelan

This comment has been minimized.

Copy link
Contributor

najamelan commented Sep 10, 2019

Have you filed an issue at tokio-dns to resolve the blocker?

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 10, 2019

Copy link
Contributor

najamelan left a comment

Just asking some questions, I haven't fully reviewed everything.

I do feel this lacks integration testing, verifying every possible scenario, but then again, tokio-tungstenite didn't have those before either.

src/compat.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/handshake.rs Outdated Show resolved Hide resolved
src/stream.rs Show resolved Hide resolved
@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 10, 2019

Just asking some questions, I haven't fully reviewed everything.

I do feel this lacks integration testing, verifying every possible scenario, but then again, tokio-tungstenite didn't have those before either.

Will look at add at least a couple more once I get something that seems like it might work.

@dbcfd dbcfd force-pushed the dbcfd:tokio2 branch from d7efd25 to d72a2d5 Sep 12, 2019
@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 12, 2019

@najamelan @application-developer-DA

Everything compiling, existing tests passing. Examples upgraded as well. Will be seeing what I can come up with for tests.

@CryZe

This comment has been minimized.

Copy link

CryZe commented Sep 15, 2019

This doesn't build without the tls feature. In particular here: https://github.com/dbcfd/tungstenite-rs/blob/expose-machine/src/client.rs#L49

error[E0432]: unresolved import `error`                                                                    
  --> C:\Users\Christopher Serr\.cargo\git\checkouts\tungstenite-rs-0e70558ccb2effd7\02684b4\src\client.rs:49:9
   |
49 |     use error::{Error, Result};
   |         ^^^^^ help: a similar path exists: `crate::error`
   |
   = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>

error[E0432]: unresolved import `stream`
  --> C:\Users\Christopher Serr\.cargo\git\checkouts\tungstenite-rs-0e70558ccb2effd7\02684b4\src\client.rs:50:9
   |
50 |     use stream::Mode;
   |         ^^^^^^ help: a similar path exists: `crate::stream`
   |
   = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 16, 2019

@CryZe This was actually an issue in tungstenite and is fixed in snapview/tungstenite-rs#73. cargo update and this branch should build now.

@CryZe

This comment has been minimized.

Copy link

CryZe commented Sep 16, 2019

Nice, another issue I ran into was that the pin-project 0.4.0-alpha.10 pulled in here is causing a cargo update failure when the new hyper is also in the dependency graph. But I don't think it's reasonable to ask for you to downgrade to their version. That's a general problem with all these alpha versions because they are all semver incompatible (to the point where you can't even have a dependency on multiple, even if they don't interact with each other). Although maybe one can specify 0.4.0-alpha.*? Idk, I haven't tried, but maybe that would be better.

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 16, 2019

Matching hyper for now, since one of the reasons for this work was to use it in warp.

@CryZe

This comment has been minimized.

Copy link

CryZe commented Sep 16, 2019

If I'm sending two messages before trying to receive any I get:

thread 'main' panicked at 'assertion failed: !self.context.is_null()', C:\Users\Christopher Serr\.cargo\git\checkouts\tokio-tungstenite-42c59291ea04b35c\bea935e\src\compat.rs:44:13 
@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 19, 2019

@CryZe Added a test which reproduces this behavior, attempting to determine cause and fix it.

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 19, 2019

The multiple write failure is due to tungstenite write_message calling flush. This means that Sink cannot be used for send, since it does not include a context.

@CryZe

This comment has been minimized.

Copy link

CryZe commented Sep 20, 2019

That seems to have fixed the bug for me :)

@CryZe

This comment has been minimized.

Copy link

CryZe commented Sep 23, 2019

Is there a reason this doesn't use Sink for sending messages?

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Sep 23, 2019

@CryZe Sink cannot be used because tungstenite write_message is also calling flush. Sink write does not provide a context, so no context is present when write_message then calls flush.

One possibility is to separate the the write and flush into separate functions in tungstenite (that write_message uses), so then we could implement Sink in tokio-tungstenite.

@dbrgn

This comment has been minimized.

Copy link

dbrgn commented Oct 1, 2019

Async/await is now available on Beta, so we don't even need nightly anymore! 🎉

https://blog.rust-lang.org/2019/09/30/Async-await-hits-beta.html

@CryZe

This comment has been minimized.

Copy link

CryZe commented Oct 2, 2019

Yeah looks like tokio and futures have released versions that are compatible with the beta, so I'd appreciate if this PR would be updated to those versions as well.

I did the changes locally here: CryZe@d4e5872

@cere42

This comment has been minimized.

Copy link

cere42 commented Oct 8, 2019

Just wanted to say I'm pretty excited about this PR, I've been trying the most recent version out for a side project the last few days. It's just very smooth integration, I'd say currently this is shaping up to become the go-to for anyone building a websocket based application using async/await.

The only issue I've noticed so far is due to not implementing Sink in this version, as mentioned above - it means you can't split the stream into Sender and Receiver because StreamExt::split requires Self: Sink, which can be a killer for some applications

@anlumo

This comment has been minimized.

Copy link

anlumo commented Oct 8, 2019

Can someone point me to a way to read and write to the same Websocket with this implementation without Sink? Both poll_next and send need a &mut self, so I can't do both without splitting.

@agalakhov

This comment has been minimized.

Copy link
Member

agalakhov commented Oct 8, 2019

It is doable via .into_future() of the Stream trait for reading. The whole is likely to be called from unfold() or loop_fn(). This technique is useful for implementing state machines.

@anlumo

This comment has been minimized.

Copy link

anlumo commented Oct 8, 2019

I just discovered that it does work when I remove my call to map on the stream. However, I need that to be able to select on two different streams with different item types. The idea is that I have an futures::channel::mpsc as the incoming message channel from other parts of the program, so these other parts can communicate with the websocket.

I also can't use forward, since that requires a Sink as well.

Seems like this is one of the applications where the Sink issue is a killer, as @skaufhold mentioned.

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Oct 8, 2019

@skaufhold I will look about adding a split method to this, so that Sink is not required.

@anlumo It will look something like this

while let Some(try_msg) = websocket_stream.next().await {
    let msg = try_msg.expect("Failed to receive message");
    sender.send(msg);
    if let Some(resp) = receiver.next().await {
        websocket_stream.send(resp).await.expect("Failed to receive message");
    }
}

You can also do something like

while let Some(try_msg) = websocket_stream.next().await {
  let msg = try_msg.expect("Failed to receive message");
  let response = transform(msg);
  websocket_stream.send(response).await.expect("Failed to send response");
}

The examples (especially autobahn) show this off better https://github.com/snapview/tokio-tungstenite/pull/68/files#diff-9da84c2001004ed41d861e7b1fc0850fR37

@mcseemk

This comment has been minimized.

Copy link

mcseemk commented Oct 8, 2019

I just discovered that it does work when I remove my call to map on the stream. However, I need that to be able to select on two different streams with different item types. The idea is that I have an futures::channel::mpsc as the incoming message channel from other parts of the program, so these other parts can communicate with the websocket.

I also can't use forward, since that requires a Sink as well.

Seems like this is one of the applications where the Sink issue is a killer, as @skaufhold mentioned.

I use the following workaround:

let (mut ws_stream, _) = match connect_async(...).await {
    Ok(x) => x,
    Err(e) => {}
};

let mut heartbeat = Interval::new(Duration::from_secs(3));

'inner: loop {
    let either1 = select(receive_channel.next(), select(heartbeat.next(), ws_stream.next())).await;
    match either1 {
        Either::Left((msg, _)) => {
            // received message via channel, process it
        }
        Either::Right((either2, _)) => {
            match either2 {
                Either::Left(_) => {
                    // heartbeat, do whatever
                }
                Either::Right((msg, _)) => {
                    // websocket message received, process it
                }
            }
        }
    }
}
@nemasu

This comment has been minimized.

Copy link
Contributor

nemasu commented Dec 3, 2019

Hello @dbcfd , I'm fiddling with the server and client examples.
The description says it broadcasts to all connected clients, but it seems to only echo messages back to the client that sent it.

@nemasu

This comment has been minimized.

Copy link
Contributor

nemasu commented Dec 5, 2019

Hello @dbcfd , I made a PR to update the server example. Seems like you need to hit return in the client terminal(s) before any new messages show up, I'm pretty sure that's the clients fault though.

@CryZe

This comment has been minimized.

Copy link

CryZe commented Dec 10, 2019

What's the status of this?

@bstrie

This comment has been minimized.

Copy link

bstrie commented Dec 10, 2019

I believe that the acceptance of this PR was blocked on the next stable hyper release? That's just happened ( https://seanmonstar.com/post/189594157852/hyper-v013 ), so are there any remaining issues here?

Danny Browning
This reverts commit dd9cad6.
@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Dec 10, 2019

@bstrie Nope :D

@dbcfd

This comment has been minimized.

Copy link
Contributor Author

dbcfd commented Dec 10, 2019

@application-developer-DA ready for review/merge

Cargo.toml Outdated Show resolved Hide resolved
Danny Browning
Cargo.toml Outdated Show resolved Hide resolved
@@ -21,7 +21,7 @@ stream = []
log = "0.4"
futures = "0.3"
pin-project = "0.4"
tokio = "0.2"
tokio = { version = "0.2", default-features = false, features = ["io-util"] }

This comment has been minimized.

Copy link
@bstrie

bstrie Dec 12, 2019

Nit, but tokio's Cargo.toml has:

# Include nothing by default
default = []

so default-features = false is a no-op.

This comment has been minimized.

Copy link
@CryZe

CryZe Dec 12, 2019

Well that's assuming they won't add some. But I guess that's somewhat unlikely.

@vorot93

This comment has been minimized.

Copy link
Contributor

vorot93 commented Dec 19, 2019

@application-developer-DA Any chance this could be merged ASAP?

@kellytk

This comment has been minimized.

Copy link
Contributor

kellytk commented Dec 23, 2019

What, if anything, could be done to progress this?

@bstrie

This comment has been minimized.

Copy link

bstrie commented Dec 23, 2019

I would suspect that the maintainers are reasonably predisposed during the holidays, although I suppose it wouldn't hurt to ping @agalakhov as well.

@agalakhov

This comment has been minimized.

Copy link
Member

agalakhov commented Dec 23, 2019

I'll check this during the holidays.

@application-developer-DA

This comment has been minimized.

Copy link
Author

application-developer-DA commented on 5a50cf8 Dec 27, 2019

NB: The "chat server" was missing in the tokio2 branch, what was there in examples is some sort of a echo server, which I re-wrote in a slightly different way. I also plan to add an equivalent of the original server.rs (chat server) example which we had before (this does not necessary have to be part of this PR though, it can be done after merge as well).

@vorot93

This comment has been minimized.

Copy link
Contributor

vorot93 commented Dec 28, 2019

@application-developer-DA while examples are definitely important, I strongly suggest merging this as is if it works. There is too much downstream blocked from upgrading to Tokio 0.2 because this PR has not been merged yet.

@agalakhov

This comment has been minimized.

Copy link
Member

agalakhov commented Dec 30, 2019

Checked this partially. This pull request requires more cleanup to be merged. I'm working on it. Sorry, it can't be merged in its current state.

@vorot93

This comment has been minimized.

Copy link
Contributor

vorot93 commented Jan 3, 2020

@agalakhov If it's in the working state it could be possible to merge as is and keep iterating in subsequent commits. You can slap an -alpha tag if you feel uncomfortable about the current quality of the port. At this point some working release is much better than no release at all!

@agalakhov

This comment has been minimized.

Copy link
Member

agalakhov commented Jan 3, 2020

I have to check for some potential security and stability issues first. Especially handshake code has to be carefully checked. Now it is much better than initially (no more unsafe), but I'm still suspicious about some places. I just don't want to see any CVE filed.

@agalakhov

This comment has been minimized.

Copy link
Member

agalakhov commented Jan 7, 2020

Ok, I checked this. While handshake submodule could be improved, I did not found any security related problems yet. It seems to be safe to merge. Thank you.

@agalakhov agalakhov merged commit 2474ba4 into snapview:master Jan 7, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.