Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Syncronize cancellation in ReadAsyncCompletesIfFlushAsyncCanceledMidFlush#31365

Merged
pakrym merged 1 commit intodotnet:masterfrom
pakrym:pakrym/tighten-the-test
Jul 25, 2018
Merged

Syncronize cancellation in ReadAsyncCompletesIfFlushAsyncCanceledMidFlush#31365
pakrym merged 1 commit intodotnet:masterfrom
pakrym:pakrym/tighten-the-test

Conversation

@pakrym
Copy link

@pakrym pakrym commented Jul 25, 2018

https://github.com/dotnet/corefx/issues/31349

Synchronizing cancellationTokenSource.Cancel(); loop with FlushAsync decreases amount of iterations required 10x on my machine.

Should make the test more stable.

@pakrym pakrym requested a review from ahsonkhan July 25, 2018 17:35
@ahsonkhan
Copy link

decreases amount of iterations required 10x on my machine.

Should make the test more stable.

Does this guarantee the test will always pass or do you expect to see intermittent failures due to timeouts going forward? If so, what would be the course of action then?

return;
}

resetEvent.Reset();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand why this re-ordering was necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So resetEvent is Set when reader completes so canceller doesn't block on WaitOne.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So resetEvent is Set when reader completes

Do you mean Reset? And why not move it to the very end of the loop (after the call to AdvanceTo)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean Set. So canceller can loop and exit.

@pakrym
Copy link
Author

pakrym commented Jul 25, 2018

Does this guarantee the test will always pass or do you expect to see intermittent failures due to timeouts going forward? If so, what would be the course of action then?

Unfortunately, it's inherently racy test.

do you expect to see intermittent failures due to timeouts going forward?

There was one failure right now I expect this PR to significantly speed test up.

If so, what would be the course of action then?

Caping the infinite loop, but that would lower the chance of reproing the original bug.

@pakrym pakrym merged commit dd4a308 into dotnet:master Jul 25, 2018
pakrym added a commit to pakrym/corefx that referenced this pull request Jul 27, 2018
@karelz karelz added this to the 3.0 milestone Aug 21, 2018
pakrym added a commit to pakrym/corefx that referenced this pull request Aug 21, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants