snapview / tungstenite-rs

Lightweight stream-based WebSocket implementation for Rust.
Apache License 2.0
1.93k stars 221 forks source link

Get stream on handshake error #51

Open canislupaster opened 5 years ago

canislupaster commented 5 years ago

I'm using mio and tungstenite, and it would be nice to have a way to discard the handle when being refused a handshake:

match mid.handshake() {
    Ok(x) => *space = SocketSpace::Connected(x),
    Err(HandshakeError::Interrupted(mid)) => *space = SocketSpace::Connecting(mid),
    Err(HandshakeError::Failure(err)) => {
       discard(mid.stream?! halp)
    }
}
daniel-abramov commented 5 years ago

I think it's possible, but it would require some changes in tungstenite-rs. If I got your request right, there is a change in HandshakeRole required, so that it returns not just an error when something fails, but something like (Error, HandshakeRole::InternalStream). Was that the thing you needed?

NB: If you just need to drop the stream, it will be dropped anyways, as mid.handshake() takes self by move.

lemunozm commented 3 years ago

Hi!

What is the state of this Issue? Is there any idea in mind to return a HandshakeError::Failure(Error, HandshakeRole::InternalStream)?

I'm in a situation where I need to deregister the stream from the poll registry if the handshake fails. Since the HandshakeError::Failure is not returning the stream, I'm not able to deregister it. Any workaround?

Thanks!

daniel-abramov commented 3 years ago

What is the state of this Issue?

I have not yet got acknowledgement from the reporter if the behavior that I described in the previous comment is the one that he would expected. There has been no progress on this issue so far.

I'm in a situation where I need to deregister the stream from the poll registry if the handshake fails. Since the HandshakeError::Failure is not returning the stream, I'm not able to deregister it. Any workaround?

So you're using it in conjunction with mio? Won't it get unregistered once the socket is dropped? I'm asking because we actually do provide tokio-tungstenite which relies on tungstenite-rs as its core and it seems to work fine (tokio uses mio under the hood).

lemunozm commented 3 years ago

Thanks for the notification. Yeah, I'm using it with mio.

Until I know from the mio's documentation, it's on the side of the user to call deregister(). If you remove the TcpStream without deregistering it you will leave the entry of the file descriptor of the socket into the OS poll, more or less like a "leak memory".

I took a look into the tokio's TcpStream and it doesn't seem that it calls the deregister() when it drops. Neither the mio's TcpStream.

EDIT: I took a deeper look and I can confirm that a tokio's TcpStream is deregistering when it drops (concretely, the PollEvented entity do that job). Although this works for the tokio's one, anyone who can use other TcpStream as mio, will get the "leak".

daniel-abramov commented 3 years ago

Any workaround?

Based on your comments, the only [temporary] dirty hack that comes to my mind is to wrap the TcpStream into another structure, implement Drop (or any other trait if required) and pass it to tungstenite-rs as tungstenite-rs is type agnostic (it just requires the stream to implement Read and Write).

leptonyu commented 2 years ago

If the input stream is not tcpstream but other layers based on it. We may hard to use Drop.

Can you provide another callback for such error? So developer can use this callback to customize error response.