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

Get stream on handshake error #51

Open
canislupaster opened this issue Dec 15, 2018 · 6 comments
Open

Get stream on handshake error #51

canislupaster opened this issue Dec 15, 2018 · 6 comments

Comments

@canislupaster
Copy link

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
Copy link
Member

daniel-abramov commented Aug 8, 2019

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.

psmit pushed a commit to psmit/tungstenite-rs that referenced this issue Nov 14, 2019
@lemunozm
Copy link

lemunozm commented May 9, 2021

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
Copy link
Member

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
Copy link

lemunozm commented May 10, 2021

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
Copy link
Member

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
Copy link

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.

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

No branches or pull requests

5 participants