Mir

Merge lp://qastaging/~afrantzis/mir/fix-1391488-basic-thread-pool-race into lp://qastaging/mir

fix-1391488-basic-thread-pool-race
Merge into development-branch

Proposed by Alexandros Frantzis on 2014-11-20

Status:

Merged

Approved by:

Kevin DuBois on 2014-11-20

Approved revision:

no longer in the source branch.

Merged at revision:

2086

Proposed branch:

lp://qastaging/~afrantzis/mir/fix-1391488-basic-thread-pool-race

Merge into:

lp://qastaging/mir

Diff against target:

97 lines (+43/-6)

1 file modified

src/server/thread/basic_thread_pool.cpp (+43/-6)

To merge this branch:

bzr merge lp://qastaging/~afrantzis/mir/fix-1391488-basic-thread-pool-race

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-11-20
Robert Carr (community)			Approve on 2014-11-20
Alberto Aguirre (community)			Approve on 2014-11-20
Alan Griffiths		2014-11-20	Approve on 2014-11-20
Review via email: mp+242348@code.qastaging.launchpad.net

Commit message

server: Fix race in BasicThreadPool

Ensure a task is removed from the task queue before notifying that the task is
done. Otherwise, scheduling new tasks based on these notifications may cause
unnecessary threads to be created.

Description of the change

server: Fix race in BasicThreadPool

Ensure a task is removed from the task queue before notifying that the task is
done. Otherwise, scheduling new tasks based on these notifications may cause
unnecessary threads to be created.

This MP replaces std::packaged_task<> with a custom Task class that provides separate operations for task execution and notifications.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-20:

52 + auto task = std::move(tasks.front());
53 lock.unlock();
54 - task();
55 + task.execute();
56 lock.lock();
57 tasks.pop_front();

Is it really safe to unlock between the "std::move(tasks.front());" and the "tasks.pop_front();"?

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-20:

FAILED: Continuous integration, rev:2078
http://jenkins.qa.ubuntu.com/job/mir-ci/2184/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/213
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/173/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/213
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/213/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3478
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16056

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2184/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-11-20:

> Is it really safe to unlock between the "std::move(tasks.front());" and the "tasks.pop_front();"?

It is: Worker::operator()() is the only place we manipulate tasks.front(), and it's only invoked once (as a thread functor). tasks.push_back() can't affect the validity of our front task, since we are moving from it. It was fine before too when we were taking a reference, since we are using a deque.

Actually, we can still keep using a reference by moving task.notify_done() before tasks.pop_front() (it makes no difference since we are under lock at that point).

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-11-20:

Unrelated CI failure: java.io.IOException: Failed to mkdirs: /var/lib/jenkins/workspace/mir-vivid-amd64-ci