Another option to fix potential HTTP/2 vio stall #6409
Conversation
|
[ci approve clang-analyzer] |
1 similar comment
|
[ci approve clang-analyzer] |
| if (this->_write_vio_event) { | ||
| this->_write_vio_event->cancel(); | ||
| } | ||
| this->_write_vio_event = this_ethread()->schedule_in(this, retry_delay, event, &write_vio); |
There was a problem hiding this comment.
If I understand this correctly, we don't need to pass write_vio because it always use THE write_vio that Http2Stream has, right?
Is retry_delay required? Should we change signal_read_event to use it too (on maybe another PR)?
Edit: I found signal_read_event already use retry_delay on this PR.
masaori335
left a comment
masaori335
left a comment
There was a problem hiding this comment.
If we need an async mechanism, rescheduling events to itself like this is an option.
Actually, I doubt the necessaries of the async mechanism, but we can deal with it later.
| reentrancy_count++; | ||
| if (e == cross_thread_event) { | ||
| if (e == _read_vio_event) { | ||
| this->signal_read_event(e->callback_event); |
There was a problem hiding this comment.
It's better to set _read_vio_event nullptr before signal_read_event() call. Otherwise, _read_vio_event->cancel() will be called when the mutex could not be locked again.
There was a problem hiding this comment.
[nitpicking] e->callback_event is same to event, right? Using event looks clear in these event handlers.
|
[approve ci clang-analyzer] |
zwoop
added
the
OnDocs
|
Cherry-picked to v9.0.x branch. |
zwoop
removed
the
OnDocs
5 participants
Another approach to address the issue identified in PR #6367. This PR does reschedule in the event that the continuation lock is unavailable. But it schedules to itself rather than to the continuation. At the top of the Http2Stream main handler, it looks for the read and write_vio events and attempts to call the read or write handler again using the event stored away in in the event's callback event.