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

Makes `write()` return `Err` when stream is closed #87

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@icefoxen
Contributor

icefoxen commented Nov 10, 2018

...instead of panicking.

Basically, I have a server that accepts a client, opens a stream to it,
sends a message to it, and closes the stream. By accident I had the
client trying to send a message back on the same stream, which panicked,
on the write(), 'cause it was already closed. This makes the write
return Err instead.

This doesn't QUITE work right still, I think it messes up the state
in the server side somehow. If I run a server, then run a client
the client properly says "that stream is closed" on the write.
But when I run it again without restarting the server then the client
never manages to connect to the server again, and times out.
The server never even sees an incoming connection.

This is still a step up for me, so I'm submitting this PR. XD

Oh, also, I don't know what error code the error should be.

Makes `write()` return `Err` when stream is closed
...instead of panicking.

Basically, I have a server that accepts a client, opens a stream to it,
sends a message to it, and closes the stream.  By accident I had the
client trying to send a message back on the same stream, which panicked,
on the `write()`, 'cause it was already closed.  This makes the write
return `Err` instead.

This doesn't QUITE work right still, I think it messes up the state
in the server side somehow.  If I run a server, then run a client
the client properly says "that stream is closed" on the write.
But when I run it again without restarting the server then the client
never manages to connect to the server again, and times out.
The server never even sees an incoming connection.

This is still a step up for me, so I'm submitting this PR.  XD
@Ralith

This comment has been minimized.

Collaborator

Ralith commented Nov 10, 2018

Thanks for the PR! Can you clarify what you mean by "[the server] closes the stream?" Bidirectional QUIC streams are like TCP connections, in that the two halves can be closed independently of eachother, so if the server closes its send half, the client should still be able to freely write in the other direction until it closes that itself. The only way for a peer to close the half of a stream which a remote peer is writing to is by explicitly calling stop on the stream, or by dropping the steam (which just implicitly calls stop). This will produce a Stopped error on the next write call.

In general, my current feeling is that it's correct for quinn to panic on things that are always a result of a programming error in the local application, although it should never be possible for any remote (mis)behavior to cause a correct local application to panic.

@icefoxen

This comment has been minimized.

Contributor

icefoxen commented Nov 12, 2018

I would really prefer my libraries didn't decide when my program needs to panic, unless it's a incredibly unrecoverable situation such as a mutex being poisoned. Especially when the function returns a Result already.

I'll try to come up with a more precise repro.

@Ralith

This comment has been minimized.

Collaborator

Ralith commented Nov 13, 2018

In gitter, @djc made a good case for panicing on internal errors only, so I think I'm okay with switching to that paradigm.

@Ralith

While I'd like to find out more about your unexpectedly closed stream, we've deemed this PR reasonable in and of itself, so let's go ahead and get it ready to merge.

@@ -2603,7 +2603,7 @@ impl Connection {
.streams
.streams
.get_mut(&stream)
.expect("stream already closed")
.ok_or(WriteError::Stopped {error_code: 0})?

This comment has been minimized.

@Ralith

Ralith Nov 16, 2018

Collaborator
Suggested change Beta
.ok_or(WriteError::Stopped {error_code: 0})?
.ok_or(WriteError::Blocked)?

This way we don't need to invent an error_code (which would normally come from the peer application), and the documentation/Display impl are even kind of accurate! There's also actually already precedent for this when a write is attempted after the connection is closed.

e: On further consideration, this might be gratuitously weird due to the handling of Blocked in the high-level bindings. Maybe better to introduce a new case in the enums?

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