You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/03/04 19:31:52 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7578: Adapt to continuation changes in write event handling
shinrich opened a new pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-868691472
@vmamidi is going to review this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-809407648
This change is also in PR #7622. Ran into this problem while testing H2 to origin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-790953035
[ci approve clang-analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] vmamidi commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
vmamidi commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-829646324
I am unable to think of use case where continuation is changed. Can you give an example?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] vmamidi commented on a change in pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#discussion_r623452751
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -431,6 +432,9 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
if (towrite != ntodo && buf.writer()->write_avail()) {
if (write_signal_and_update(VC_EVENT_WRITE_READY, vc) != EVENT_CONT) {
return;
+ } else if (c != s->vio.cont) { /* The write vio was updated in the handler */
+ write_reschedule(nh, vc);
+ return;
}
Review comment:
Shouldn't we check for continuation change all the time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-790980182
@vmamidi can you please look at this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] closed pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-831552713
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-790953277
[approve ci clang-analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-926283485
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-790975929
[approve ci clang-analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich removed a comment on pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich removed a comment on pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#issuecomment-790953035
[ci approve clang-analyzer]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a change in pull request #7578: Adapt to continuation changes in write event handling
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7578:
URL: https://github.com/apache/trafficserver/pull/7578#discussion_r625387481
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -431,6 +432,9 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
if (towrite != ntodo && buf.writer()->write_avail()) {
if (write_signal_and_update(VC_EVENT_WRITE_READY, vc) != EVENT_CONT) {
return;
+ } else if (c != s->vio.cont) { /* The write vio was updated in the handler */
+ write_reschedule(nh, vc);
+ return;
}
Review comment:
Good point, I see another call in this function that should be checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org