Skip to content

TS-4614: avoid e->schedule_in for dummy event #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
oknet wants to merge 1 commit into from
Closed

TS-4614: avoid e->schedule_in for dummy event #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions iocore/net/UnixNet.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}

Copy link
Member

@shinrich shinrich Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
2 changes: 1 addition & 1 deletion iocore/net/UnixNetVConnection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

@shinrich shinrich Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@oknet oknet Jul 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@oknet oknet Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e->schedule_in() makes InactivityCop stop running if the e is InactivityCop's callback Event.
because schedule_in() set period to 0.

Copy link
Member

@shinrich shinrich Jul 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the change makes sense now.

}

Expand Down