-
Notifications
You must be signed in to change notification settings - Fork 851
Adapt to continuation changes in write event handling #7578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
[approve ci clang-analyzer] |
1 similar comment
|
[approve ci clang-analyzer] |
|
@vmamidi can you please look at this? |
|
This change is also in PR #7622. Ran into this problem while testing H2 to origin. |
|
I am unable to think of use case where continuation is changed. Can you give an example? |
| } else if (c != s->vio.cont) { /* The write vio was updated in the handler */ | ||
| write_reschedule(nh, vc); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check for continuation change all the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I see another call in this function that should be checked.
|
The continuation associated with the vio may be changed in the handler if do_io_write is performed in the handler. The case I ran into was in the h2 to origin branch. But another example shows up for example if an origin connection is returned to the session pool. In that case, the do_io_write continuation would be changed from the HttpSM to the HttpSessionPool. |
|
@vmamidi is going to review this. |
|
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community. |
Another issue I encountered with the H2 to origin processing. But this one could affect other code paths as well.
In the write_to_net_io(), we deliver the WRITE_READY event to the write_vio continuation for processing. It is possible in the handlers, the write_vio is adjusted so the continuation is changed. In that case, the write processing should be restarted to make the appropriate changes to the new version of the write_vio.
In my original version, I just added the continuation check to the proceeding if. So if the write_signal_and_update returned something other than EVENT_CONT or the continuation in the vio changed, we just returned. However, in that case we would lose the WRITE_READY. By rescheduling the WRITE_READY is delivered to the new continuation if there is more data to be processed than handled by the original continuation.