ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor

[pitrou]

No description provided.

[pitrou]

cc @westonpace

[pitrou]

I've checked locally that this fixes the TSan failures.

[pitrou]

Submitted crossbow build: https://github.com/ursacomputing/crossbow/branches/all?query=build-226

[lidavidm]

So the issue was that we might be in the middle of SpawnReal on an external thread while the main thread was destructing the state (e.g. it exited early due to an error or something)?

[pitrou]

Yes.

[pitrou]

Hm, wait, I shouldn't unlock before notifying in MarkFinished().

[lidavidm]

Additionally, since we're handing around Executor* instead of shared_ptr or something, there's still the possibility that an I/O thread tries to use a dangling reference, right? The main thread has to make sure to block and wait for all I/O threads it spawns under all conditions.

[pitrou]

Well, the idea is that the top-level task waits for all dependent tasks to finish, I think.

[lidavidm]

True, it's just a potentially easy pitfall (though it comes with the territory I suppose).

[pitrou]

Things may get tricky if cancellation or early exit is involved indeed...

[lidavidm]

Seems Crossbow built Arrow master, not this branch. I threw up https://github.com/lidavidm/crossbow/actions/runs/748753207

[lidavidm]

The crossbow job passes!

[pitrou]

Github Actions: https://github.com/pitrou/arrow/actions?query=branch%3AARROW-12379-tsan++
Travis-CI: https://travis-ci.com/github/pitrou/arrow/builds/223062391

[lidavidm]

Seemed the integration build timed out in Java but everything else passed.

[westonpace]

Thanks @pitrou . A few thoughts (nothing worrisome):

Why deque instead of queue? I don't mind the change. I just want to make sure I'm not missing some piece of knowledge.

Things may get tricky if cancellation or early exit is involved indeed...\

Cancellation should still flush the task queue. Early exit (via bad Status) should still flush the task queue. It is important we do because in both cases the remaining tasks might need to do some cleanup to handle the error. Exceptions/assertions might not but we're already in crash territory there.

[pitrou]

It's only for consistency, since deque is used for the threadpool executor.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12379