core.sync.event.Event set() behaviour change

[denizzzka]

Hi!

It looks like Event's bool wait() set() method may do a disservice. It returns false if Event struct isn't initialized and I think what this is error-prone way.

Related post: https://forum.dlang.org/post/oqjezxtysbkmuowaeamu@forum.dlang.org

Of course it would be better to change Event completely, but then reverse compatibility will break.

It is good when .set() does not able to called at uninitialized object (by Event ctor) and when wait() is a real infinity "wait", not returning a binary value which nobody checks.

[dlang-bot]

Thanks for your pull request and interest in making D better, @denizzzka! 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 run digger -- build "master + druntime#3273"

[thewilsonator]

please:
- reference a bugzilla issue
- use a more descriptive PR/issue/commit message title
- a tests

[denizzzka]

• reference a bugzilla issue

For discussion purpose or should it be an issue? it's not a bugfix, just change of a confusing non-obviousness.

My motivation: I fell for this non-obvious behavior and decided to get rid of it (and also spotlight probably unnecessary .set() call inside of gc.d).

[rainers]

IMO setIfInitialized() is not a very useful primitive. Instead, initialize() should report "success" via its return value, or there should be a way to test this, e.g. some valid-method.

IIRC this was omitted for Event because none of the other synchronization types had it.

[rainers]

I'd be ok with the assert-version if it was used from the start, but I'm not sure it's worth changing and risking to break code. Please note that that the assert will not be tested by many programs that use the precompiled druntime library.

[denizzzka]

We have way (like version directive) to check if druntime precompiled or not?
If so, this would allow us to make a appropriate condition here and gradually all users would fix their programs.

[rainers]

I don't think execution should get here if startScanThreads failed to start any threads. I suspect that your system is single-core, so startScanThreads could return false to update doParallel in fullcollect in that case.

[denizzzka]

I suspect that your system is single-core

Yes, and I see on it what evStart still isn't initialized here.

I understand that this is not an error, but it looks very strange and confusing.

[rainers]

The error is that markParallel is called at all after startScanThread has aborted early on a single-core system.

[denizzzka]

The error is that markParallel is called at all after startScanThread has aborted early on a single-core system.

Ah, yes, this is that's just fine :-) I have not yet ported all calls (for example, low-level threads is created, but not joined)
For such cases (assert?) check is needed here.

[denizzzka]

Can we call onOutOfMemoryError() or onInvalidMemoryOperationError from Event? It is can be called inside of nothrow code.
It would be a more convenient way not to forget about errors handling.

[denizzzka]

Instead, initialize() should report "success" via its return value

initialize() also used in ctor() which can't return value

[rainers]

initialize() also used in ctor() which can't return value

Then I'd add a bool isInitialized() or similar.

[rainers]

Can we call onOutOfMemoryError() or onInvalidMemoryOperationError from Event? It is can be called inside of nothrow code.
It would be a more convenient way not to forget about errors handling.

Other synchronization primitives throw a SyncError, that should be ok here, too.

[denizzzka]

Too big changes. IMHO, it is necessary to make a new structure and current Event should be declared as deprecated.