Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
| if (connection_manager_.codec_->protocol() < Protocol::Http2) { | ||
| // For HTTP/2 there are still some reset cases where details are not set. | ||
| // For HTTP/1 there shouldn't be any. Regression-proof this. | ||
| ASSERT(filter_manager_.streamInfo().responseCodeDetails().has_value()); |
There was a problem hiding this comment.
@snowp @alyssawilk I suspect there is some race here with onEvent(RemoteClose) and deferred delete. We may not be able to detect remote close if we log immediately at the end of the stream. Any thoughts?
There was a problem hiding this comment.
If we're going to move logs around, think I'd like this assert removed to before we log (we want to make sure details are set before stream info is shipped to access logs)
I don't think we should have to set DownstreamRemoteDisconnect in two places, and I'm not sure what race there would be. If we get a normal end_stream set, the stream should finish inline and not be lingering around to pick up and I'd think any subsequent disconnects wouldn't be registered?
There was a problem hiding this comment.
I'm getting test failures if I don't update stream info in the destructor. Might be a test issue again. Is it possible to get onEvent(RemoteClose) once stream is scheduled to be defer-deleted? If not, then it is likely just another unrealistic test.
There was a problem hiding this comment.
yeah, I'd guess it's the tests being badly ordered but you should be able to check with gdb or envoy logs.
There was a problem hiding this comment.
What I mean with the race is the following sequence:
- Socket is read ready, data read, calling onData() and and encodeHeaders() in HCM.
- encodeHeaders() ends stream, triggers log().
- Socket is remote-closed.
It seems that we need to process the file events before triggering log() but I'm not sure how to hold onData() for that.
There was a problem hiding this comment.
ah, in that case in production I think the details would basically be set on encodeHeaders (like the router will when proxying from upstream, or the HCM will when sendLocalReply is called) so maybe switch to that?
There was a problem hiding this comment.
Thanks for explaining. That works indeed.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
Windows failure looks like a flake in ext_authz test. Can someone re-kick windows CI? |
|
/retest should work for Azure pipelines now |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
alyssawilk
left a comment
There was a problem hiding this comment.
Awesome, thanks!
Tagging Matt for cross-company approval and cc @snowp as this might help the HCM ownership tangle (at least would in the original PR)
|
Oh actually already tagged for Snow, so I'll leave that as is :-P |
call log() before onDestroy(), as well as populate final stream info details at the time of log() rather than the deferred destruction. Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Clara Andrew-Wani <candrewwani@gmail.com>
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: call log() before onDestroy(), as well as populate final stream info details at the time of log() rather than the deferred destruction.
Additional Description:
Risk Level: low (might affect timings of events)
Testing: unit
Docs Changes: none
Release Notes: none
Fixes #12616
Fixes #12840