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/05/25 20:14:29 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7386: Clean up producer more regularly
shinrich opened a new pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386
Discussion in the issue. This PR will clean up the producer and call back into the HttpSM producer handler.
This closes issue #7385
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r544409477
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
I agree that consumer never reads it, but NetVC/CacheVC may still try to read the outstanding data and signal the continuation that VIO is holding and may result in crash if that continuation is already cleared.
----------------------------------------------------------------
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] zwoop commented on pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-849786487
@shinrich @bryancall @vmamidi I've marked this for Project 9.0.x, I think VJ needs this, and sounds like it's been tested thoroughly ?
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-848228763
Looks like this may fix the leaking server sessions identified in issue #7471
--
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] ywkaras merged pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
ywkaras merged pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-813705569
Closing this. We seem to be running fine with 9.1.x without this change.
--
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] maskit commented on pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-848431024
@shinrich Can you rebase the branch onto master just in case? We no longer have the Warning but GitHub shows the diff somehow.
Although I am not sure if this is a right approach, I confirmed that this stops the leak reported on #7471.
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r544412076
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -91,7 +91,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
- Warning("read: Closing orphaned vc %p", vc);
+ Warning("read: Closing orphaned vc vc=%p cont=%p event=%d", vc, vc->read.vio.cont, event);
Review comment:
This is the normal scenario as the state machine may decide to go away for several reasons, but iocore might still keep the VC for clearing the events. Please let me know if I am missing something.
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r542824574
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
At this point, we may still have still have data left to read from the producer, right? How can we send an VC_EVENT_READ_COMPLETE to the producer?
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -91,7 +91,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
- Warning("read: Closing orphaned vc %p", vc);
+ Warning("read: Closing orphaned vc vc=%p cont=%p event=%d", vc, vc->read.vio.cont, event);
Review comment:
Is orphaned VC a new name for inactivity cop? Why are we throwing a warning for a typical inactivity cop scenario?
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r639157020
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
We have been running this commit in our 9.0.x since the end of 2020 without issue.
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-848792567
Rebased
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r543367835
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -91,7 +91,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
- Warning("read: Closing orphaned vc %p", vc);
+ Warning("read: Closing orphaned vc vc=%p cont=%p event=%d", vc, vc->read.vio.cont, event);
Review comment:
Only throwing a warning in case there was no continuation associated with the VC. Should not normally happen.
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r542824574
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
At this point, we may still have data left to read from the producer, right? How can we send an VC_EVENT_READ_COMPLETE to the producer?
--
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 closed pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich closed pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386
--
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] zwoop commented on pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-849902110
Cherry-picked to v9.0.x branch.
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r547493119
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -91,7 +91,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
- Warning("read: Closing orphaned vc %p", vc);
+ Warning("read: Closing orphaned vc vc=%p cont=%p event=%d", vc, vc->read.vio.cont, event);
Review comment:
In my experience this is a pretty rare occurrence, but I'm ok with taking out the warning message.
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-848888594
[approve ci autest]
--
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r543368505
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
If the consumer is write_complete, it doesn't matter if there was more data on the producer, the consumer will never read it. The tunnel will be stuck forever.
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r547492870
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
We should not leave around any vio's pointing to the continuation after we have freed the 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] vmamidi commented on a change in pull request #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
vmamidi commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r544409477
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1336,16 +1336,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c)
c->producer->read_success = true;
// Go ahead and clean up the producer side
if (p->alive) {
- p->alive = false;
- if (p->read_vio) {
- p->bytes_read = p->read_vio->ndone;
- } else {
- p->bytes_read = 0;
- }
- if (p->vc != HTTP_TUNNEL_STATIC_PRODUCER) {
- // Clear any outstanding reads
- p->vc->do_io_read(nullptr, 0, nullptr);
- }
+ producer_handler(VC_EVENT_READ_COMPLETE, p);
Review comment:
I agree that the consumer never reads it. However, NetVC/CacheVC may still try to read the outstanding data and signal the continuation that VIO is holding and may result in a crash if that continuation is already cleared.
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-773650436
@vmamidi Can you take a look at this again? Do you still have issues?
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#issuecomment-773650436
@vmamidi Can you take a look at this again? Do you still have issues?
----------------------------------------------------------------
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 #7386: Clean up producer more regularly
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #7386:
URL: https://github.com/apache/trafficserver/pull/7386#discussion_r639157391
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -91,7 +91,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc)
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
Debug("inactivity_cop", "event %d: null read.vio cont, closing vc %p", event, vc);
- Warning("read: Closing orphaned vc %p", vc);
+ Warning("read: Closing orphaned vc vc=%p cont=%p event=%d", vc, vc->read.vio.cont, event);
Review comment:
Pushed another commit to remove the warning.
--
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