TS-4614: avoid e->schedule_in for dummy event #766
Conversation
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
[approve ci] |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/385/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/281/ for details. |
| #endif | ||
| e->schedule_in(HRTIME_MSECONDS(net_retry_delay)); | ||
| #endif | ||
| return EVENT_CONT; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 do 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.
There was a problem hiding this comment.
The e->schedule_in() makes InactivityCop stop running if the e is InactivityCop's callback Event.
because schedule_in() set period to 0.
There was a problem hiding this comment.
Ok, the change makes sense now.
|
This is a InactivityCop minor issue. @bryancall |
|
@shinrich Should we land this? Also, is this a back port candidate? |
|
Yes, I'll go ahead and merge this up. Might consider backporting. Stopping the period on the inactivty cop seems like a bad thing. |
* asf/master: (44 commits) TS-4614: avoid e->schedule_in for dummy event. This closes apache#766. Use devtoolset-3 for ATS 7 and later, when available Updated some build instructions TS-4311 Removes support for SPDY completely TS-4683 Adds better error handling on config problems Add TSQA tests for https SRV records TS-4615 set SRV scheme based on next_hop_scheme Add some initial SRV tsqa tests TS-4622 use port from SRV response for origin connections Revert "TS-4622 Use port from SRV response when connecting to origin (apache#773)" TS-4622 Use port from SRV response when connecting to origin (apache#773) TS-4688 handle DNS compression labels in SRV responses (apache#812) cachekey/pattern.h: clang format TS-4667 Uses the WKS in the gzip plugin TS-4653: esi plugin - disable HTTP_COOKIE variable by default and implement a whitelist mechanism to allow the specified cookies for it Original code and idea contributed by Chris Rohlf TS-4680: thread safe initialization in TS*DirGet() functions TS-4652 Fixes log field initialization, previously crippled TS-4595: TSRuntimeDirGet TS-4689 Assert if Machine UUID init fails TS-4674: Remove useless assert statement (apache#809) ...
* Adds Au test bad_server_crash.test.py * set cause_of_death_errno only if it's the default value) Co-authored-by: Walt Karas <wkaras@yahooinc.com>
4 participants
No description provided.