Mailing-List: contact dev-help@thrift.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@thrift.apache.org
Date: Mon, 4 Sep 2017 21:41:02 +0000 (UTC)
From: =?utf-8?Q?Bu=C4=9Fra_Gedik_=28JIRA=29?= <jira@apache.org>
To: dev@thrift.apache.org
Message-ID: <JIRA.13006129.1474357542000.23854.1504561262799@Atlassian.JIRA>
In-Reply-To: <JIRA.13006129.1474357542000@Atlassian.JIRA>
References: <JIRA.13006129.1474357542000@Atlassian.JIRA> <JIRA.13006129.1474357542578@jira-lw-us.apache.org>
Subject: [jira] [Commented] (THRIFT-3932) C++ ThreadManager has a rare
 termination race
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
archived-at: Mon, 04 Sep 2017 21:41:18 -0000


    [ https://issues.apache.org/jira/browse/THRIFT-3932?page=3Dcom.atlassia=
n.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D161=
52958#comment-16152958 ]=20

Bu=C4=9Fra Gedik commented on THRIFT-3932:
-------------------------------------

Hi [~jking3]. Do you know if there are any release plans that would contain=
 this fix? Is there a publicly available resource that talks about release =
plans? Thanks!

> C++ ThreadManager has a rare termination race
> ---------------------------------------------
>
>                 Key: THRIFT-3932
>                 URL: https://issues.apache.org/jira/browse/THRIFT-3932
>             Project: Thrift
>          Issue Type: Bug
>          Components: C++ - Library
>            Reporter: Bu=C4=9Fra Gedik
>            Assignee: James E. King, III
>             Fix For: 0.10.0
>
>         Attachments: thrift-patch
>
>          Time Spent: 17h
>  Remaining Estimate: 0h
>
> {{ThreadManger::join}} calls {{stopImpl(true)}}, which in turn calls {{re=
moveWorker(workerCount_);}}. The latter waits until {{while (workerCount_ !=
=3D workerMaxCount_)}}. Within the {{run}} method of the workers, the last =
thread that detects {{workerCount_ =3D=3D workerMaxCount_}} notifies {{remo=
veWorker}}. The {{run}} method has the following additional code that is ex=
ecuted at the very end:
> {code}
>     {
>       Synchronized s(manager_->workerMonitor_);
>       manager_->deadWorkers_.insert(this->thread());
>       if (notifyManager) {
>         manager_->workerMonitor_.notify();
>       }
>     }
> {code}
> This is an independent synchronized block. Now assume 2 threads. One of t=
hem has {{notifyManager=3Dtrue}} as it detected the {{workerCount_ =3D=3D w=
orkerMaxCount_}} condition earlier. It is possible that this thread gets to=
 execute  the above code block first, {{ThreadManager}}'s {{removeWorker}} =
method unblocks, and eventually {{ThreadManager}}'s {{join}} returns and th=
e object is destructed. When the other thread reaches the synchronized bloc=
k above, it will crash, as the manager is not around anymore.
> Besides, {{ThreadManager}} never joins its threads.
> Attached is a patch.
> {panel:title=3DResolution Details|borderStyle=3Ddashed|borderColor=3D#ccc=
|titleBGColor=3D#F7D6C1|bgColor=3D#FFFFCE}
> A fair number of things were improved with this pull request:
> # There are three monitors used for signaling; two of them use the same m=
utex and then the workerMonitor is using its own. I made all three use the =
same mutex.
> # I replaced all calls to Synchronize with Guard on the mutex_ so it is a=
 bit clearer that there is one lock protecting everything.
> # The worker run loop expires tasks as it encounters them. Now we only ca=
ll removeExpiredTasks if there's no room during add.
> # I found that when worker threads are removed they are not joined; if a =
thread is not detached we now join those threads.
> # I added better documentation and simplified the Worker::run() a little =
so it is easier to understand, but the logic is the same. idle_ was not use=
d and removed.
> # I found that remove(Task) simply wasn't implemented at all! so I added =
code for that and tests.
> # Release mode tests of thread manager would not fail on error because th=
ey use assert() to verify results! If we only do release builds in CI, thes=
e tests may have been silently failing for a long time.
> # ThreadManager::join() is unnecessary, as stop() will join worker thread=
s (as will removeWorker) depending on the ThreadFactory's detached disposit=
ion.
> # Cleaned up some technical debt from the TServerFramework refactoring:
> # Cleaned up some things in the different platform thread factories. Some=
 abstractions were unnecessary.
> # Reduced overall time on concurrency test to about 30 seconds on my dev =
system while increasing the reliability and improving test coverage signifi=
cantly. I ran it 100 times in a loop.
> # Added tests for most of the ThreadManager APIs.
> # Tested with posix, std, and boost threads on linux.
> # Tested on Windows.
> # Fixed a math error in time calculation that was present on Windows (wit=
h VS2010) which made expiring tasks impossible (unit test proved the issue =
and the fix).
> # Re-enable unit tests in the appveyor build that are no longer unstable.
> {panel}


--
This message was sent by Atlassian JIRA
(v6.4.14#64029)