StephenCleary / AsyncEx

A helper library for async/await.
MIT License
3.49k stars 358 forks source link

Enforce that monitor is entered when calling `Pulse` and `PulseAll` or remove requirement? #260

Open WalkerCodeRanger opened 2 years ago

WalkerCodeRanger commented 2 years ago

The doc comments for AsyncMonitor.Pulse() and AsyncMonitor.PulseAll() say "The monitor MUST already be entered when calling this method." It doesn't explain what will actually happen if that isn't done.

Testing shows that calling Pulse or PulseAll outside of the monitor does actually release any waiters. I'm not sure what the requirement that it must be entered is about. It seems like changes should be made in line with one of three options:

  1. If there isn't actually a requirement to enter the monitor, then update the docs and doc comments to reflect that.
  2. If there is actually a requirement to enter the monitor
    • check that the monitor has been entered and throw an exception if it hasn't (the System.Threading.Monitor class throws SynchronizationLockException when Pulse or PulseAll is called when the calling thread does not own the lock for the specified object. I understand that doesn't make sense for AsyncMonitor because it doesn't support reentrancy, but there should be some equivalent check.
    • update the doc comments to explain why this restriction is in place
    • if possible enforce that Pulse and PulseAll are only called from the code the entered the monitor. The way to do this would be to move them from AsyncMonitor to disposable object returned by EnterAsync
  3. If there is actually a requirement to enter the monitor, but nothing can be done to enforce this
    • update the doc comments to explain why this restriction is in place and what will go wrong if it isn't followed.
StephenCleary commented 2 years ago

It can't be enforced via exceptions; it would be possible but too expensive in the async world. Enforcing by API is not a bad idea, though.

Thanks for writing this up!