You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/06/29 09:52:14 UTC
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/766
TS-4614: avoid e->schedule_in for dummy event
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver TS-4614
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/766.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #766
----
commit ea21ed77e71d1052b50ad6fe6337c89d4f4d05d2
Author: Oknet Xu <xu...@skyguard.com.cn>
Date: 2016-06-29T08:25:38Z
TS-4614: avoid e->schedule_in for dummy event
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/766
@shinrich Should we land this? Also, is this a back port candidate?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/766#discussion_r72139223
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1139,8 +1139,8 @@ UnixNetVConnection::mainEvent(int event, Event *e)
(write.vio.mutex && wlock.get_mutex() != write.vio.mutex.get())) {
#ifdef INACTIVITY_TIMEOUT
if (e == active_timeout)
-#endif
e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+#endif
return EVENT_CONT;
--- End diff --
Ok, the change makes sense now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/766
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/766#discussion_r68942583
--- Diff: iocore/net/UnixNet.cc ---
@@ -682,8 +682,9 @@ NetHandler::_close_vc(UnixNetVConnection *vc, ink_hrtime now, int &handle_event,
// create a dummy event
Event event;
event.ethread = this_ethread();
- vc->handleEvent(EVENT_IMMEDIATE, &event);
- ++handle_event;
+ if (vc->handleEvent(EVENT_IMMEDIATE, &event) == EVENT_DONE) {
+ ++handle_event;
+ }
}
}
--- End diff --
I guess this makes a bit more sense. As far as I can tell the handle_event variable is only used for a Debug statement, so the change isn't crucial one way or another.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/766
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/766#discussion_r71065621
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1139,8 +1139,8 @@ UnixNetVConnection::mainEvent(int event, Event *e)
(write.vio.mutex && wlock.get_mutex() != write.vio.mutex.get())) {
#ifdef INACTIVITY_TIMEOUT
if (e == active_timeout)
-#endif
e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+#endif
return EVENT_CONT;
--- End diff --
The e->schedule_in() makes InactivityCop stop running if the e is InactivityCop's callback Event.
because schedule_in() set period to 0.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/766#discussion_r71065565
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1139,8 +1139,8 @@ UnixNetVConnection::mainEvent(int event, Event *e)
(write.vio.mutex && wlock.get_mutex() != write.vio.mutex.get())) {
#ifdef INACTIVITY_TIMEOUT
if (e == active_timeout)
-#endif
e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+#endif
return EVENT_CONT;
--- End diff --
yes, currently default build ats without INACTIVITY_TIMEOUT defined.
that's means the UnixNetVConnection::mainEvent(int event, Event *e) is callbacked from InactivityCop::check_inactivity(int event, Event *e) and/or NetHandler::_close_vc().
The Event *e is InactivityCop's Event when it is callbacked from InactivityCop::check_inactivity(int event, Event *e). and the Event is a perorid Event.
thus we don't need to to e->schedule_in() for it.
The Event *e is a dummy Event when it is callbacked from NetHandler::_close_vc(). To schedule a dummy Event will lead memory leak and ATS crash.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/766
Yes, I'll go ahead and merge this up. Might consider backporting. Stopping the period on the inactivty cop seems like a bad thing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #766: TS-4614: avoid e->schedule_in for dummy eve...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/766#discussion_r71044678
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1139,8 +1139,8 @@ UnixNetVConnection::mainEvent(int event, Event *e)
(write.vio.mutex && wlock.get_mutex() != write.vio.mutex.get())) {
#ifdef INACTIVITY_TIMEOUT
if (e == active_timeout)
-#endif
e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+#endif
return EVENT_CONT;
--- End diff --
This appears to be the more critical fix. It looks like a standard reschedule if you don't get the lock. But it appears that we only do this if the event is an active timeout (if INACTIVITY_TIMEOUT is set). It seems that we are not currently building with INACTIVITY_TIMEOUT defined (at least I'm not). So this change would cause the reschedule to never happen. So if you don't get the lock the first time, you will never process the event.
I'm not following the description in the JIRA. Is the concern that we are double freeing an event object?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/766
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/281/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/766
This is a InactivityCop minor issue. @bryancall
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #766: TS-4614: avoid e->schedule_in for dummy event
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/766
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/385/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---