Don't leak incoming bytes when we race incoming data and close

[swankjesse]

We had a bug where a race between FramingSource.receive() and
FramingSource.close() could cause newly-received bytes to be
absent from the flow control window. If this happens enough then
eventually the connection will stall.

[dave-r12]

Good eye. I think these are tricky to write test cases for. Just wondering if that's possible.

I wanted to confirm my understanding of the events.

1. Server sends back some data for connection X on stream 3.
2. Http2Reader thread reads that data and stores it in the buffer for stream 3.
3. Application (or something) closes stream 3.
4. Application attempts to read the data for stream 3.

[swankjesse]

Here’s the bug scenario:

HTTP/2 Reader thread:

1. Server sends back some data for connection X on stream 3. Stream 3 is still open so this is okay.
2. Http2Reader thread reads that data and stores it in the receiveBuffer for stream 3.

Application Thread

3. Closes stream 3. This discards the readBuffer and tells the connection how many bytes were discarded for flow control.
4. Removes stream 3 from the list of receiving streams.

HTTP/2 Reader thread

5. Http2Reader moves data from receiveBuffer to readBuffer. Nobody will ever read this data because the stream is already closed.

[swankjesse]

I don’t think we can write tests for these, but we could improve this code by adding fast-fail invariants. This bug is insidious because it leaks a few bytes on the connection flow control window, and that only shows up hours after when the connection stalls.

[yschimke]

We could probably add some logging of terminal states or similar so dev report in the wild. But could be issue noisy.

[yschimke]

Especially nice catch, if this is closing 2 or 3 of the trickiest and noisiest bug reports we've had.