http/2: fix deferred reset behavior #1145
Conversation
The logic was broken and would not invoke the deferred reset stream cleanup logic in all cases. This now does that and the test coverage has been improved for both the client and server case. Fixes #1138
|
@lyft/network-team @htuch @alyssawilk @andrewvd this should fix your issue if you have any way to compile and test the change. |
|
Thanks! We will test it and provide feedback |
| // We must still call sendPendingFrames() in both the deferred and not deferred path. This forces | ||
| // the cleanup logic to run which will reset the stream in all cases if all data frames could not | ||
| // be sent. | ||
| parent_.sendPendingFrames(); |
There was a problem hiding this comment.
Could we made the "pending deferred reset" a parameter to sendPendingFrames() now? It seems like pending_deferred_reset_ is acting as effectively a parameter for the pending case behavior, sendPendingFrames() is the only consumer.
There was a problem hiding this comment.
Unfortunately we can't, because sendPendingFrames() will return immediately if we are currently dispatching read data (interleaving read/write is not possible w/ nghttp2). So we still need the flag for the currently dispatching case.
| EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)) | ||
| .WillOnce(InvokeWithoutArgs([&]() -> void { | ||
| // Start a response inside the headers callback. This should not result in the client | ||
| // seeing any headers as the stream should already be reset on the other side. |
There was a problem hiding this comment.
Can this be made explicit by verifying decodeHeaders is not called on a response decoder?
There was a problem hiding this comment.
Implicitly it is because of the EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); expectation above. I will add a comment.
|
|
||
| // This will send a request that is bigger than the initial window size. When we then reset it, | ||
| // after attempting to flush we should reset the stream and drop the data we could not send. | ||
| TestHeaderMapImpl request_headers; |
There was a problem hiding this comment.
Why don't you need to flush SETTINGS as in the client case above?
There was a problem hiding this comment.
I will add more comments.
|
@alyssawilk @htuch I simplified the tests based on your comments. It took me a while to figure out what was going on and how to test it and I think what I had was a remanent or initial attempts. This should be more clear. |
The logic was broken and would not invoke the deferred reset stream
cleanup logic in all cases. This now does that and the test coverage
has been improved for both the client and server case.
Fixes #1138