Skip to content

Conversation

@shinrich
Copy link
Member

@shinrich shinrich commented Feb 7, 2020

Breaking apart the PR in #6401.

This PR concentrates on fixing the read_vio.ndone tracking so the read_complete is sent to the HttpTunnel in a timely manner. Needs to be applied with PR #6408 to address the crashing issue.

@shinrich shinrich added this to the 10.0.0 milestone Feb 7, 2020
@shinrich shinrich self-assigned this Feb 7, 2020
}
myreader->writer()->dealloc_reader(myreader);

if (frame.header().flags & HTTP2_FLAGS_DATA_END_STREAM) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we treating read_vio.ntodo() == 0 the same as getting a data end stream flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protecting ourselves from ill-written clients that do not send DATA_END_STREAM. We have seen such clients in our production data stream. In that case they are sending Content-length, so we can still send READ_COMPLETE when all of the data is written.

@maskit
Copy link
Member

maskit commented Feb 8, 2020

While I understand ATS should protect itself, I don't think adding the condition is a fundamental solution. What are we going to do if we support trailing header? I assume trailing header would be written to the same VIO. Can we do READ_COMPLETE twice?

I don't understand how lack of READ_COMPLETE event causes a crash, but I guess the logic that assumes READ_COMPLETE is already received is perhaps what we should fix.

If this is a bandaid and we are going to take another approach when we support trailing header, it should be stated on the code or be filed as an issue.

@zwoop zwoop added the OnDocs This is for PR currently running, or will run, on the Docs ATS server label Feb 11, 2020
@zwoop zwoop removed the OnDocs This is for PR currently running, or will run, on the Docs ATS server label Feb 25, 2020
@shinrich
Copy link
Member Author

shinrich commented Mar 4, 2020

I think we picked PR #6409 and #6404 to address this, so closing this option

@shinrich shinrich closed this Mar 4, 2020
@masaori335 masaori335 removed this from the 10.0.0 milestone Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants