Fix a race condition in ToAsyncEnumerable on cancellation by slavashar · Pull Request #1855 · dotnet/reactive

Hi @idg10,

It’s been a few years since I last looked at this, but I wanted to address some of your concerns:

  • The issue is related to a race condition, which makes it challenging to create a reliable unit test without the risk of false positives. However, this functionality is already covered in the existing tests.
  • Currently, on cancellation, ObservableAsyncEnumerable calls Dispose() on itself, which I found unconventional, especially since it handles some signaling logic afterward. This is problematic as the instance is still in use by the consumer (the enumerator pattern). Instead, we should unsubscribe and signal that it has completed.
  • Dispose() should only be called by the consumer (the enumerator pattern) and not internally.
  • The cancellation of the enumerable is controlled by _cancellationToken. If it is raised after the check on line 60, I believe we should allow the last item to be produced consistently before canceling the enumerable in the next cycle.
  • Checking for null reliably without locking is tricky because three variables (_signal, _values, and _error) might be set or accessed by different threads.

Let me know if you have any further thoughts.