HTTP/2 send goaway with error condition must close tcp connection

[Scottmitch]

section 5.4.1:

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

We should update the life cycle manager's goAway documentation such that upon successful go away the channel must be closed. The Htto2ConnectionHandler should also be updated to implement this.

[Scottmitch]

@ejona86 - Thanks for brining my attention to this!

@nmittler - FYI.

[Scottmitch]

@nmittler - I think this means we can get away without keeping history for #3557. We can just send a connection error in these "bad" conditions and it is OK if state is "invalid" because we are closing the connection. WDTY?

[nmittler]

@Scottmitch I'm not sure this gets us out of the woods. Consider this scenario:

1. Client sends HEADERS
2. Client sends RST_STREAM
3. Server receives HEADERS
4. Server sends response HEADERS
5. Client receives response HEADERS and throws a connection error since the stream no longer exists.

This doesn't involve GO_AWAY at all.

[Scottmitch]

Yes there will be legitimate race conditions to deal with however I would like to explore if the following code (or something similar) is "sufficient" to ignore a frame due to these sequencing / race condition scenarios:

private boolean streamMayHaveExisted(int streamId) {
    return connection.remote().createdStreamId(streamId) && streamId < connection.remote().nextStreamId();
}

If we feel that we should be returning errors after some "time" then we should keep some form of history. Possibly just in a limited capacity as suggested by @buchgr #3557 (comment).

[nmittler]

@Scottmitch wouldn't you need to check both endpoints, not just remote?

[buchgr]

@Scottmitch +1

[Scottmitch]

@nmittler - That is a good point. I will think through the details but I'll submit a PR with this type of "simple" checks...and also explore "simple history" for problem streams.

[buchgr]

@Scottmitch since you seem to have looked into the details of the spec regarding this issue, could you please summarize in a short post what's the required behavior for every case and what's no required (just in like 5 bullet points). I have kinda lost track of when we have to send a RST_STREAM frame, when we have to close the connection and when we have to ignore a frame and such ... thanks!

[nmittler]

@Scottmitch @ejona86 @buchgr I was thinking how we might add the concept of history to the current design. It seems like Http2LifecycleManager might be a good landing place for this. It could have a history method that returns an interface that can be queried:

interface ConnectionHistory {
    enum StreamHistory {
        CLOSED_NORMALLY,
        RESET_SENT,
        RESET_RECEIVED
    }

    // Not sure if we still need this if we're maintaining closeState.
    boolean streamMayHaveExisted(int streamId);
    StreamHistory streamHistory(int streamId);
}

The implementation will be in the Http2Connection which can use @ejona86's scheme for swapping maps, described in #3557 (comment).

This should help us resolve the issues in the encoder and decoder WRT closed streams.

Stream History and Flow Control ... :/

The only problem that still remains is to figure out what flow controllers should do for absent streams.

Right now the flow controller interfaces require a Http2Stream, which makes sense so that we don't have to perform multiple map lookups on the same stream. I think the only case we need to consider is receiving an inbound DATA frame for a closed stream, which would mean that the only methods that need to handle closed streams are receiveFlowControlledFrame, consumeBytes, and unconsumedBytes.

To simplify things, we could make StreamHistory implement Http2Stream but be backed by very little data (e.g. streamId + close state). This would get us past the interface of the flow controller, but there would be no properties (i.e. flow state). We could change the interface spec of the Http2LocalFlowController, requiring that it support closed streams with no flow state attached. This all seems a little hacky, however ... likely needs more thought.

[buchgr]

@nmittler that all sounds good to me. No allocations yay :-). don't have a good solution for the flow controller atm either. all a bit hacky :\

[nmittler]

@Scottmitch I'd could try throwing together a PR for this to see what it looks like ... WDYT?

[Scottmitch]

@nmittler - Sorry for the delayed response. I'm still wondering if we need the history. I'm going to switch gears back into this topic shortly but I think keeping history for every stream may be overkill. Is the primary motivator for keeping history to be able to know when receiving a frame associated with a stream we don't have an object allocated for an error or not? If so I would still like to explore the approach described in #3653 (comment) (ignore frames for streams that "may" have existed). If we can tolerate "ignoring" frames for streams that "may have existed" this may reduce the amount of work we have to do for the majority of the use cases. WDYT?

[Scottmitch]

I have a PR pending for this...