Fix issue 17037 - std.concurrency has random segfaults

[WalterWaldron]

https://issues.dlang.org/show_bug.cgi?id=17037

[dlang-bot]

Thanks for your pull request and interest in making D better, @WalterWaldron! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	17037	major	std.concurrency has random segfaults

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#5004"

[John-Colvin]

A comment would be nice to explain why doing a collection helps here as it's not immediately obvious.

[WalterWaldron]

std.concurrency is designed around having a global variable (scheduler) set once at the outset. There is no synchronization for this variable and the implementation does not appear to support changing it on the fly.

However we need to test with both implementations of Scheduler: ThreadScheduler and FiberScheduler.

This function waits until it is the only thread before modifying scheduler (i.e. it's a mutual exclusion hack.)
Collection helps because threads can wait on the finalization action of other threads (e.g. waiting for OwnerTerminated exceptions initiated by static ~this.)

[John-Colvin]

Thanks for the explanation, I actually meant a comment in the source. I suggest something like // wait for all other threads to terminate, using GC.collect to trigger finalizers which may terminate threads (e.g. OwnerTerminated or LinkTerminated) at the top of the loop.

[WalterWaldron]

I know, I was giving the explanation as interim to updating the PR.

[WalterWaldron]

Updated according to feedback.

[MetaLang]

@wilzbach in the same vein as #5515 (comment) why hasn't the bot suggested a reviewer?

[WalterWaldron]

@MartinNowak ping please! this has been open for 7 months!
I'm pinging you since you're slated to be std.concurrency code owner.

[wilzbach]

@wilzbach in the same vein as #5515 (comment) why hasn't the bot suggested a reviewer?

Same answer as in #5515 (comment) (we turned the feature of due to too much noise), but #5573 looks very promising.

[MetaLang]

This has been all green for awhile now which I think should be a pretty good indicator that it at least shouldn't break anything, and if it does we can revert it. Unfortunately Martin is a pretty busy guy so it's hard to say when he'll get to this.

[WalterWaldron]

This has been all green for awhile now which I think should be a pretty good indicator that it at least shouldn't break anything, and if it does we can revert it.

It can't break code because it only modifies the unittests. My changes are:

• Add changeScheduler inside version(unittest) block: This function is a hack to make changing the scheduler in unittests more sane.
• Modify problematic unit test: failures in other threads are reported to the main thread which executes the assertion:
• • this avoids depending on exceptions thrown in other threads to signal test failure.
• • this allows blocking the main thread until test completion to avoid interference

[MetaLang]

It can't break any code because it only modifies the unittests

What I was getting at is any unittest build would fail if one of the tests was broken. It's pretty common for people to run the full test suite for Phobos locally, and would also break the auto tester.

[JackStouffer]

I don't see any problems with this. I'll leave this open for two or three more days and merge if no one has any more comments.

[MartinNowak]

How exactly does the race condition manifest?

Add changeScheduler inside version(unittest) block: This function is a hack to make changing the scheduler in unittests more sane.

From a first look, I'd say we should make the tests more sane instead.

[MartinNowak]

That looks frightening? Do we really only send Owner/LinkTerminated messages when the thread object get's collected? Sounds horribly unreliable as the other peer might hold some (implicit) reference to the thread.
If so we should add some onThreadExit hook to core.thread or wrap the thread function with some scope (exit) guard.

[WalterWaldron]

Do we really only send Owner/LinkTerminated messages when the thread object get's collected?

They get sent when the module destructor is run for threads, and via scope(exit) for fibers, so the comment I added must be wrong.

I have look at the code again (it's been so long since I made this PR) to see whether this was just a hack for force bad tests to hang (instead of random failures,) or whether it was necessary.

[MartinNowak]

Somewhat unclear, is this really the last life-signal of thread being spawned?

[WalterWaldron]

Yes, I made it so that the test result (failure/success) was communicated back to the main thread instead of relying on exceptions being re-thrown (like it had been previously.)

[WalterWaldron]

How exactly does the race condition manifest?

Typically it was failing like this:

@property ref ThreadInfo thisInfo() nothrow
{
    if (scheduler is null) // scheduler is modified from non-null to null after this test
        return ThreadInfo.thisInfo;
    return scheduler.thisInfo; // Scheduler is null
}

scheduler was concurrently modified by the unit test code.

From a first look, I'd say we should make the tests more sane instead.

The problem is that the code being tested references the global variable (__gshared Scheduler scheduler;) and we want to test both ThreadScheduler and FiberScheduler implementations. The code assumes scheduler will be set once and left.

[WalterWaldron]

I've removed the changeScheduler hack, it wasn't necessary (it must have been left over from when I was debugging because it would make bad tests hang instead of pass & randomly segfault.)

[WalterWaldron]

I think it's still necessary to serialize changing the scheduler, however I don't think the GC.collect part should be present (Thread.pause until other threads have exited.)
With the modified test code, I think the following could still happen:

Main Thread              |    Thread 1    |    Thread 2
                         |  static ~this  | 
                         |    cleanup     |    Owner/Link Terminated
assert(receiveOnly...    |                | 
scheduler = new ...      |                |
scheduler.start( ...     |                |
assert(receiveOnly...    |                |    static ~this
scheduler = null         |                |    thisInfo()      (data race on scheduler)

[Imperatorn]

Is this still "frightening". More review needed?

Addressed