Skip to content

Fix strand.cpp + test which reveals (with help of FaultyThreadsTSan) the need for a fix

Explanation of the test and why it fails without changes:

In the test I have a futures::Submit which repeatedly links-unlinks to the CancelState of the outer futures::Start future which is Cancelled/Awaited in another futures::Submit.

I) else if(expected != nullptr) Consider the following execution:

T0

futures::Start (let's call it "S1") links with cancel::Never generated by Await. 

futures::Never instantly calls `Forward` on "S1". 

Forward` eventually enters StrandStateBase::ResetHandler with expected == nullptr. 

T1

Underlying future enters "DDos loop": inner futures::Start, "S2", links to the "S1".

T0

StrandStateBase::ResetHandler call reads address of "S2" into curr.

T1

"S2" completes and unlinks from "S1" using the StrandStateBase::ResetHandler.

Next iteration of DDos loop creates futures::Start, "S3" which links to the "S1".

T0

StrandStateBase::ResetHandler call CAS on state_ fails and now curr contains address of "S3".

WHEELS_VERIFY(!curr.IsPointer(),...) fails.

Now, expected != nullptr means that we are in the unlinking process and !curr.IsPointer() in fact must be enforced, since failing it would imply "More than 1 concurrent handlers for CancelState".

Another fix could be introducing a separate function used by Forward(Signal::Release()) to do the same as RequestCancel but with a different signal and leave StrandStateBase::ResetHandler only to the unlinking producers.

II) Since that in case of expected != nullptr we use curr when CAS fails, we need to be in hb with its' constructor. Thus we need acquire part that was missing.

Alternative would be removing else branch altogether at least in the Release build.

P.S. Changes were tested using older versions of twist and sure (GIT_TAGs from the last concurrency course) and several undocumented changes in await, all of which were removals of duplicate static. Unit tests pass. Both cancel stress tests pass as well.

Edited by E1pp

Merge request reports

Loading