4.x: Fix Delay hanging due to wrong disposable-chain management #560
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danielcweber
reviewed
Jun 1, 2018
| return this; | ||
| } | ||
|
|
||
| protected override void Dispose(bool disposing) |
There was a problem hiding this comment.
Move those two disposables and Dispose into the base class?
There was a problem hiding this comment.
The class is ugly, didn't want to accidentally break it.
There was a problem hiding this comment.
Should be good for now, might be an optimization PR for later.
danielcweber
reviewed
Jun 1, 2018
| // ScheduleDrain might have already set a newer disposable | ||
| // using TrySetSerial would cancel it, stopping the emission | ||
| // and hang the consumer | ||
| Disposable.TrySetSingle(ref _cancelable, parent._scheduler.Schedule(parent._dueTime, Start)); |
There was a problem hiding this comment.
_cancelable was as SerialDisposable before, even if this works, consider TrySetSerial throughout to reveal intention more consistently.
There was a problem hiding this comment.
That was the bug: _cancelable was SerialDisposable and SerialDisposable always forcefully overwrites and disposes the previous IDisposable it contains.
danielcweber
approved these changes
Jun 1, 2018
This was referenced Jun 6, 2018
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug in
Delaythat can lead to an unwanted cancellation of the drain loop. In addition, theIStopwatch.Elapsedshould be consistently read while holding the same lock.The bug is that the
_cancelablecan be reset by theIDisposableof the very first task starting the drain loop while that drain loop already set theIDisposablefor the next round. The first assignment then cancels the second run's cancellation token which then throws anObjectDisposedExceptionand the loop quits for good. Any queued items and terminal signals will then never be emitted.The solution is to conditionally set that first
IDisposableif the target is still null, otherwise the subsequent runs have set a newer one and nothing has to be done. The disposability is still intact in both cases.To be able to do this, the original subscription containers had to be replaced by fields so that the new atomic
Disposable.TryXmethods can be employed.In the pre-patch version, running the
DelayTest.Delay_DateTimeOffset_DefaultSchedulerwith a loop a million times and with no practical time shift (FromMilliseconds(0)) triggered the bug within the first few hundred rounds. With the patch, the same test succeeded all million rounds. I did not change the test because it would take a minute to complete and also code coverage tends to choke on such high-volume tests in my experience.Fixes: #382