Skip to content

Conversation

@shinrich
Copy link
Member

@shinrich shinrich commented Mar 4, 2021

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.

@shinrich shinrich added this to the 10.0.0 milestone Mar 4, 2021
@shinrich shinrich self-assigned this Mar 4, 2021
@shinrich
Copy link
Member Author

shinrich commented Mar 4, 2021

[approve ci clang-analyzer]

1 similar comment
@shinrich
Copy link
Member Author

shinrich commented Mar 4, 2021

[approve ci clang-analyzer]

@bryancall bryancall requested a review from vmamidi March 4, 2021 22:09
@bryancall
Copy link
Contributor

@vmamidi can you please look at this?

@shinrich
Copy link
Member Author

This change is also in PR #7622. Ran into this problem while testing H2 to origin.

@shinrich shinrich mentioned this pull request Apr 5, 2021
@vmamidi
Copy link
Contributor

vmamidi commented Apr 29, 2021

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;
}
Copy link
Contributor

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?

Copy link
Member Author

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.

@shinrich
Copy link
Member Author

shinrich commented May 3, 2021

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.

@bryancall
Copy link
Contributor

@vmamidi is going to review this.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Sep 24, 2021
@github-actions github-actions bot closed this Oct 1, 2021
@zwoop zwoop removed this from the 10.0.0 milestone Oct 7, 2021
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.

4 participants