Skip to content

Fix large download stuck with H2 priority#5835

Closed
shinrich wants to merge 1 commit intoapache:masterfrom
shinrich:fix-large-file-h2-priority
Closed

Fix large download stuck with H2 priority#5835
shinrich wants to merge 1 commit intoapache:masterfrom
shinrich:fix-large-file-h2-priority

Conversation

@shinrich
Copy link
Copy Markdown
Member

Was trying enabling priorities with HTTP/2 again. Fetching a 1.2MB file from cache would frequently (almost always) fail. It would send the first part and then stall. Eventually things would timeout and abort.

Looking at the debug messages, we saw that many data frames were sent. Then we would hit "No payload". Nothing more would happen on that thread until 30 seconds later when the state machine would cleanup because of inactivity timeout.

Talking with @SolidWallOfCode it appears that we need to signal WRITE_READY to the cache vc after we drain the buffer, so it will write more data into the buffer. This is done in the non-priority case.

I saw all this on our internal branch, so I am not 100% sure this is an issue in open source. But I'm heading out for a week, so I wanted to push this issue up before I left in case it is a general issue on open source master.

I saw some other irregularities with the priority processing which I don't have a fix for. Will file an issue for that.

@shinrich shinrich added this to the 9.0.0 milestone Aug 16, 2019
@shinrich shinrich self-assigned this Aug 16, 2019
@shinrich shinrich force-pushed the fix-large-file-h2-priority branch from c3a2a99 to f9a7014 Compare August 18, 2019 13:28
@shinrich
Copy link
Copy Markdown
Member Author

[approve ci autest]

@shinrich
Copy link
Copy Markdown
Member Author

[approve ci clang-analyzer]

@maskit
Copy link
Copy Markdown
Member

maskit commented Aug 21, 2019

I cannot reproduce the issue on master. Can you confirm this is an issue in public version too?

Also, there is a similar function, Http2Stream::signal_write_event. I'm not sure if this can be an alternative, but calling it in Http2ConnectionState::send_data_frames_depends_on_priority would look more reasonable than adding a similar function and call it in send_a_data_frame.

@zwoop zwoop modified the milestones: 9.0.0, 10.0.0 Aug 21, 2019
@shinrich
Copy link
Copy Markdown
Member Author

Indeed, it looks lovely on master.

@shinrich shinrich closed this Aug 26, 2019
@zwoop zwoop removed this from the 10.0.0 milestone Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants