core.thread.refactor Part 1

[baziotis]

Pinging @thewilsonator

[dlang-bot]

Thanks for your pull request and interest in making D better, @baziotis! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + druntime#2821"

[thewilsonator]

Thanks.

[thewilsonator]

std\concurrency.d(39): Error: module `thread` is in file 'core\thread.d' which cannot be read

???

[thewilsonator]

runnable/test17072.d(12): Error: template instance `dur!"msecs"` template `dur` is not defined, did you mean dup?

Not sure where this is coming from.

[baziotis]

runnable/test17072.d(12): Error: template instance `dur!"msecs"` template `dur` is not defined, did you mean dup?

Not sure where this is coming from.

I think I know. This PR is the same with the previous, except one line.
package.d does not public import time, as the previous. The old thread.d imported publicly time which seemed like a bad practice. In the previous PR I had changed the spinlock.d to include time locally and I thought this was the only place. But apparently not. I see 2 choices:
a) public import time and be done with it.
b) Fix the errors as they come (e.g. this error is not shown to me locally).
I'd go with b) unless I have missed a reason for why thread imported publicly time.

[atilaneves]

What's the justification for the refactoring? There's nothing in the description.

[thewilsonator]

#2689

[baziotis]

runnable/test17072.d(12): Error: template instance `dur!"msecs"` template `dur` is not defined, did you mean dup?

Not sure where this is coming from.

I think I know. This PR is the same with the previous, except one line.
package.d does not public import time, as the previous. The old thread.d imported publicly time which seemed like a bad practice. In the previous PR I had changed the spinlock.d to include time locally and I thought this was the only place. But apparently not. I see 2 choices:
a) public import time and be done with it.
b) Fix the errors as they come (e.g. this error is not shown to me locally).
I'd go with b) unless I have missed a reason for why thread imported publicly time.

Going with a) for now because doing b) requires changing files (at least) in DMD.

[quickfur]

This PR broke Fiber-dependent code: https://issues.dlang.org/show_bug.cgi?id=20279

[baziotis]

Well, we've got to find a way to test these things. The error seems trivial but the problem was not caught by CI. This PR was commited and reverted already once.
The old core.thread was so convoluted that such errors ought to be caught by CI if this PR is ever to be accepted successfully (and given that I don't know the code in core.thread).

[quickfur]

Maybe the way forward is to keep the PR merged, and fix any regressions as they surface. Resetting to square one just because the next minor thing broke seems like the wrong approach to me.

[baziotis]

I agree. More so, given that CI did not catch the error. So, resetting would not provide any benefit.

[quickfur]

Actually, it appears to be a problem with stale build products in my local druntime build environment. Cleaning all build products and rebuilding from scratch appears to have fixed the problem.

Sorry for the false alarm. :-(

[baziotis]

Don't worry. I'm glad it was so easy to (not even) fix. :)

[thewilsonator]

Actually I think this still managed to cause errors, as the test for issue 15779 (fibers throwing exceptions) is failing on a bunch of other PRs.

[baziotis]

Also, this passed the tests as a PR. But in the commit, it shows me it has failed Azure pipelines.

[rainers]

This has undone the changes in #2801.

[wilzbach]

I really wish we had actual reviews on PRs, but I guess the only thing left to do is to revert #2822

[quickfur]

Yikes. Is there really no way to add unittests / external tests for these cases so that they will be caught before the PR is merged?

[rainers]

The tests are there and are failing now. Unfortunately they don't fail consistently and pass sometimes depending on the alignment of the fiber stack.

[quickfur]

Ah I see. And there's no way to insert a canary value that might trigger the failure more consistently?

[rainers]

Possible, but that would need more investigations. What makes it a bit tedious is that the error happens only with Windows Server that has tightened policies regarding the exception handling.

[carun]

Just wondering if it is common to merge the PRs with failing tests.

[carun]

I think part of the problem lies with non-linear git history as well, as the reviewer could've missed the point that this has undone #2801 (thanks @rainers for pointing out).

Why not enable explicit rebase (no squash) for PR merges?

[thewilsonator]

For some reason the bot decided to merge the PR without waiting for the Azure tests to finish, let alone pass.

[baziotis]

Seb: I really wish we had actual reviews on PRs, but I guess the only thing left to do is to revert #2822

This PR is almost 3 months old (#2689) without a single review except for Nicholas'. Although, these in my opinion are CI problems and not review problems.
But I'm just mentioning it because it's not sure that the author of a PR will
have any time to continue working on it 3 months after it was first submitted.

carun: Just wondering if it is common to merge the PRs with failing tests.

Those are not actual failing tests. They're just stopped tests AFAIK. You can see the last commit passing.

Nicholas: For some reason the bot decided to merge the PR without waiting for the Azure tests to finish, let alone pass.

For me, it showed that it passed checks on the PR but then failed when it was committed.