router: fix http/2 shadow request with body.#5674
Conversation
We need to buffer the decoded data before performing on complete processing. Fixes #5646 Signed-off-by: Matt Klein <mklein@lyft.com>
|
cc @irelandKen |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Nice catch! I'm a bit unsure of the fix, so more questions than comments from me...
| // buffer limit we give up on retries and buffering. We must buffer using addDecodedData() | ||
| // so that all buffered data is available by the time we do request complete processing and | ||
| // potentially shadow. | ||
| callbacks_->addDecodedData(data, true); |
There was a problem hiding this comment.
I think this is the second time in as many weeks I've seen us have to do this workaround.
Not required for this PR but any thoughts for a change we could make to avoid this gotcha?
There was a problem hiding this comment.
Yeah this has come up a few times in slightly different contexts. I will think about this. I don't think there are any easy fixes here given the different permutations of things we need to support. The only thing I can think of quickly is to somehow indicate that a filter is of a particular type, and then the HCM would "pre-buffer." With that said, even that wouldn't work for the router filter because we buffer sometimes and not others...
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk updated |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Awesome, thanks for the fix and for the clarification!
| FakeStreamPtr upstream_request2; | ||
| ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection2)); | ||
| ASSERT_TRUE(fake_upstream_connection2->waitForNewStream(*dispatcher_, upstream_request2)); | ||
| ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); |
There was a problem hiding this comment.
Boo, one of our utilities should work for this and they totally don't.
Goes on my TODO list :-)
There was a problem hiding this comment.
Yeah sorry I thought the same thing and was going to make the utility functions work for this, but I got lazy. I will circle back if you don't get to it first.
We need to buffer the decoded data before performing on complete processing. Fixes envoyproxy#5646 Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Dan Zhang <danzh@google.com>
We need to buffer the decoded data before performing on complete processing. Fixes envoyproxy#5646 Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Matt Klein <mattklein123@gmail.com>
We need to buffer the decoded data before performing on complete processing. Fixes envoyproxy#5646 Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Fred Douglas <fredlas@google.com>
2 participants
We need to buffer the decoded data before performing on
complete processing.
Fixes #5646
Risk Level: Medium. Scary code to change but pretty confident in
the change and I added an integration test.
Testing: New integration test.
Docs Changes: N/A
Release Notes: N/A