Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4047 closed defect (fixed)

Fix concurrency issues in the TimerMgr

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The TimerMgr as it is implemented now is vulnerable to concurrency issues when the TimerMgr::setup is called from the ONE_SHOT timer callback. Such callback is executed from the main thread and it modifies the internal timers queue which is also being accessed by the worker thread to dispatch callbacks.

To ensure thread safety, the setup() and cancel() must be called in the worker thread if the thread is running.

There is a couple of other fixes to be applied too.

Subtickets

Change History (6)

comment:1 Changed 4 years ago by marcin

  • Owner set to tmark
  • Status changed from new to reviewing

The worker thread is now remains blocked until the watch socket is cleared. This is proven to be the most reliable way to avoid concurrency issues. I have ran over 3000 iterations of the TimerMgrTest with no fail.

I have also created the TimerMgrImpl class as the TimerMgr implementation became quite complex and I wanted to keep the details out of the header file, which will be included in many places.

I fixed the issues with static deinitialization fiasco between the TimerMgr and IfaceMgr by storing shared pointer to the IfaceMgr as long there are registered timers.

ChangeLog:

XXXX.	[bug]		marcin
	Fixed issues with threads concurrency in the TimerMgr.
	(Trac #4047, git abcd)

comment:2 follow-up: Changed 4 years ago by tmark

  • Owner changed from tmark to marcin

I think doing it as a PIMPL is a good idea.
No major issues, mostly minor nits.


General:

There are no unit tests of ScopedTrue?, though I realize most of
this is tested in lib/util. Maybe not worth it.


General:

Is there a test which specifically verifies that calling stopThread()
will unblock a waiting worker thread? We need one.


timer_mgr_unittest.cc

Rather than calling TimeMgr::instance in many places, why not add a
TimerMgrPtr? timer_mgr_ member (not a reference) to TimerMgrTest? and
initialize it during test construction?

This would also ensure TimerMgr? exists util after test deconstruction.


timerMgr.cc -

void controlledStopThread(const bool run_pending_callbacks);

Naming this "controlledStopThread" implies there is an "uncontrolled"
variant. There doesn't seem to be so I would rename this "stopThread".


timerMgr.cc -

In the commentary for TimerMgrImpl::timerCallback, you should make it more
prominent that this method BLOCKS until the main thread does its thing.


timerMgr.h -

/ The @c TimerMgr::timerCallback

This section needs to discuss blocking aspect of this method. To me this
is a significant behaviorial aspect of the API and needs to be understood.

comment:3 in reply to: ↑ 2 Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

I think doing it as a PIMPL is a good idea.
No major issues, mostly minor nits.


General:

There are no unit tests of ScopedTrue?, though I realize most of
this is tested in lib/util. Maybe not worth it.

This is internal to the TimerMgr implementation, so it is not even accessible from the unit tests. So, I am not testing it directly.


General:

Is there a test which specifically verifies that calling stopThread()
will unblock a waiting worker thread? We need one.

There is a test stopThreadWhileRunningHandlers which shedules the timer and doesn't call !IfaceMgr::receive6. This should effectively cause the worker thread to mark socket as "ready" and block. The thread is then stopped with all ready handlers to be executed before stop. The test then checks that the handler is executed. This exercises the scenario but we don't check if the thread is really blocked and such, simply because we don't have access to internals of the TimerMgr. This would require some instrumentation in the TimerMgr, which I am not sure is worth it.


timer_mgr_unittest.cc

Rather than calling TimeMgr::instance in many places, why not add a
TimerMgrPtr? timer_mgr_ member (not a reference) to TimerMgrTest? and
initialize it during test construction?

This would also ensure TimerMgr? exists util after test deconstruction.

Ok, I updated the test as suggested.


timerMgr.cc -

void controlledStopThread(const bool run_pending_callbacks);

Naming this "controlledStopThread" implies there is an "uncontrolled"
variant. There doesn't seem to be so I would rename this "stopThread".

Renamed.


timerMgr.cc -

In the commentary for TimerMgrImpl::timerCallback, you should make it more
prominent that this method BLOCKS until the main thread does its thing.

It is better now?


timerMgr.h -

/ The @c TimerMgr::timerCallback

This section needs to discuss blocking aspect of this method. To me this
is a significant behaviorial aspect of the API and needs to be understood.

I added a paragraph about blocking to the description of the TimerMgr class.

comment:4 Changed 4 years ago by tmark

  • Owner changed from tmark to marcin

Changes are fine. Please merge.

comment:5 Changed 4 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged with commit 48297af6e0443808a482536b61436a42bc6a5b38

comment:6 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.