Current handling of GOAWAY is pretty broken

[carllerche]

Right now, GOAWAY is interpreted as a signal to reset all streams. This, however, is not correct behavior.

[seanmonstar]

Handling GOAWAY is a complicated mess. A GOAWAY frame does not necessarily mean the connection is terminating (now or even that soon). It contains a reason, and a last stream ID. The connection should continue to try to process frames for any stream below the listed stream ID, and not make any new streams.

It seems a graceful shutdown involves one side sending a GOAWAY, both sides finishing up the existing streams until EOS, and then the other side sends GOAWAY, and the connection terminates.

• When receiving a GOAWAY:
  • Close the ability to create new streams
  • Drop any streams higher than the reported last stream ID. (There is no need to send a RST_STREAM, it's considered dead.)
  • Keep processing all streams lower than or equal to the last stream ID.
  • Once all streams are dropped or EOS, send GOAWAY and close.
  • Connection::poll() should then resolve:
    • If there was NO_ERROR, then Ok(Async::Ready(()))
    • Otherwise, Err(goaway_frame.reason()).
    • If there is an IO or protocol error after receiving a GOAWAY, what should be reported to the user?
      • It's possible to send multiple GOAWAY frames as well, in case graceful shutdown encountered an error.
• When we encounter a protocol error, and need to send a GOAWAY:
  • We need to determine if that error is recoverable.
    • A frame parse error, or a FRAME_SIZE_ERROR, or a COMPRESSION_ERROR, etc may mean that there is no way to continue using the connection at all. The best we can do is send a GOAWAY and then close the connection, not continuing to process frames.
    • Other errors might be recoverable, such as receiving an invalid frame type on a stream not accepting those frames. The spec says to send a GOAWAY, but we can still try to finish up any existing valid streams.
      • In this case, we can act just like when we receive a GOAWAY.

[carllerche]

When receiving a GOAWAY.

I would add that all streams higher than the reported stream ID should behave the same as if they received a RST_STREAM. Specifically, the GOAWAY reason is bubbled up to the stream handles.

If there is an IO or protocol error after receiving a GOAWAY, what should be reported to the user?

I'd say the same thing that happens if there wasn't a GOAWAY frame before hand. (probably the same thing that happens now?)

When sending

Other errors might be recoverable, such as receiving an invalid frame type on a stream not accepting those frames. The spec says to send a GOAWAY, but we can still try to finish up any existing valid streams.

Do you have links in the spec? As in, shouldn't all "recoverable" errors result in a RST_STREAM? Or are there errors that are "semi" recoverable? Aka, no new streams but existing streams can complete?

[seanmonstar]

Do you have links in the spec?

For instance, in the idle state:

Receiving any frame other than HEADERS or PRIORITY on a stream in this state MUST be treated as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

We have to send a GOAWAY, but at that point, we've already parsed a frame, so the socket is probably clear to try to do more, so we could finish up any earlier valid streams.

[carllerche]

@seanmonstar but, I don't follow where in the spec it describes that a connection error of type PROTOCOL_ERROR can still allow open streams to continue.

[seanmonstar]

Good news, the spec is confusing!

My initial understanding was wrong, connection errors should close the socket after sending GOAWAY:

After sending the GOAWAY frame for an error condition, the endpoint MUST close the TCP connection.

Confusion comes from the GOAWAY section:

The sender of a GOAWAY frame might gracefully shut down a connection by sending a GOAWAY frame, maintaining the connection in an "open" state until all in-progress streams complete.

I'm going to assume then this means GOAWAY with NO_ERROR only. Any other reason means terminate connection.

[carllerche]

@seanmonstar I don't know if I would make the NO_ERROR assumption. I'd just assume that you continue all streams until you receive an I/O error or a read() -> 0.

For example, I would assume that if a server detects many connections from a single IP, it could send GOAWAY w/ ENHANCE_YOUR_CALM w/o terminating the socket (only rejecting new streams).

[seanmonstar]

So for now then, I'll assume to try for graceful shutdown no matter the error code, and allow the remote to hang up if it didn't want to shutdown gracefully. It seems that is what the spec suggests to do.

---

As I implement this, I'm left with a couple of questions due to ambiguity, so I'll air them out here:

1. If both sides gracefully shutdown using NO_ERROR, should an Err really be returned from Connection::poll, or should it map NO_ERROR (assuming no other errors existed between receiving that goaway) as Ok(()). It feels like it should just be Ok(()), mimic EOF in Read that returns Ok(0).
2. If the remote sends a GOAWAY, but does allow some more processing of streams, when we send the returning GOAWAY, should it include the same reason code, or should it send NO_ERROR if there aren't any errors shutting down?

[carllerche]

1. I agree with your reasoning w/ NO_ERROR. A graceful shutdown with NO_ERROR should result in Ok(()).

2. So, I would guess that it should send NO_ERROR?

[seanmonstar]

I'd say GOAWAY is no longer "pretty broken". It will try to finish up streams and then close up when done. It doesn't have a way for a user to initiate graceful shutdown yet, but I'd actually say that is a different issue. I'm therefore thinking this could be closed, and new issues could opened:

• Add a way for a user to signal graceful shutdown.
• Automatically GOAWAY if the user drops all client::{Client, Stream, Body} handles (so the only thing left is the client::Connection.

There is already an open issue to add a way to tell a server to close up on idle: #69.