Skip to content

WIP Add large response header tests#7071

Closed
aunu53 wants to merge 1 commit intoenvoyproxy:masterfrom
aunu53:response
Closed

WIP Add large response header tests#7071
aunu53 wants to merge 1 commit intoenvoyproxy:masterfrom
aunu53:response

Conversation

@aunu53
Copy link
Copy Markdown

@aunu53 aunu53 commented May 24, 2019

I'm trying to add tests and upper bounds around response header size. These tests add 2^15 kb tests (our internal requirement). The http1 codec tests are fine, but the http2 tests cause codec issues I need help (probably from @mattklein123) diagnosing.

From some tweaking, I found that 2^5 kb succeeds, while 2^6 kb causes a premature connection close. This actually causes a segfault because we don't null-check the output of getStream() at (I THINK; have to double-check) http2/codec_impl.cc:528. Aside from that error-catch, there is some sort of bug in the nghttp2 code here; does anyone have experience with the nghttp2 code at all?

Signed-off-by: Auni Ahsan <auni@google.com>
@alyssawilk alyssawilk self-assigned this May 28, 2019
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented May 28, 2019

Hey @auni53 can you repro in an end to end test?

I haven't dug fully into this, but the mocks in this test are set up to auto-dispatch as data is written to the connection. Given the way this test breaks down we're sending a bunch of large header continuation frames, it could be something like

[start sending large header encode, which is broken into multiple frames]
[large block dispatched to peer]
---[peer sends reset]
---[reset dispatched to peer]
-----[reset processed by sender of large header encode]
-----[stream of large header encode is deleted]
[send of large header resumes, but stream is deleted]

Adding some error logging it looks like the connection doing the encoding of large headers is receiving data while under the stack of sendPendingFrames so my guess is the problem is something like that.

@htuch
Copy link
Copy Markdown
Member

htuch commented May 28, 2019

Please merge master to pick up #7090

@stale
Copy link
Copy Markdown

stale Bot commented Jun 4, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 4, 2019
@aunu53 aunu53 closed this Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants