hcm: handle stream reset during downstream encoding#19144
hcm: handle stream reset during downstream encoding#19144alyssawilk merged 9 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Dan Zhang <danzh@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
I'd definitely like to see this integration tested.
I believe we can enable DISABLED_AdminDrainDrainsListeners with this change, but also with some potential improvements to get the new HandleSocketFail test to work for HTTP/3?
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Also one high level question for @mattklein123
The problem here is we're writing to a closed socket, which causes us to close the connection, and reset the stream under the stack of a write, and then we were processing end stream.
I wonder if instead of immediate connection close, we'd be better off scheduling the close for the next loop to avoid closing the downstream connection under the stack of the upstream connection?
In this case, the connection was closed by QUICHE in its write call stack. I don't think we can delay the close. What we can delay is HCM's reset callback. |
Signed-off-by: Dan Zhang <danzh@google.com>
No objection from me at a high level if we need to do deferred processing, though I haven't spent the time to look at this in detail. If you want me to look at this change in more detail I can spend some time tomorrow. |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
nah I buy dan's argument that it's not worth the refactor |
Signed-off-by: Dan Zhang <danzh@google.com>
|
Integration test is added. PTAL |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
lgtm modulo one remaining nit
| @@ -3557,7 +3557,7 @@ TEST_P(DownstreamProtocolIntegrationTest, HandleSocketFail) { | |||
| socket_swap.write_matcher_->setSourcePort(lookupPort("http")); | |||
| socket_swap.write_matcher_->setWriteOverride(ebadf); | |||
| // TODO(danzh) set to true. | |||
Signed-off-by: Dan Zhang <danzh@google.com>
|
Can it be merged? |
5 participants
Signed-off-by: Dan Zhang danzh@google.com
Commit Message: when the downstream codec encodes response header/body/trailers in the same call stack as upstream decoding, which is the case for Http3, a connection could gets closed which will cause the stream to be reset. In such case, FilterManagerImpl shouldn't continue process the rest of the response, especially end_stream.
Additional Description: this contributes to the crash in #19006. This fix stops the crash, but didn't make the test pass.
Risk Level: low
Testing: new unit tests
Release Notes: guarded by envoy.reloadable_features.handle_stream_reset_during_hcm_encoding