Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToObservable: Do not call MoveNext with an already canceled CancellationToken #104

Merged
merged 2 commits into from
Aug 3, 2016
Merged

ToObservable: Do not call MoveNext with an already canceled CancellationToken #104

merged 2 commits into from
Aug 3, 2016

Conversation

danielcweber
Copy link
Collaborator

@danielcweber danielcweber commented Jun 10, 2015

This would happen whenever an IObserver.OnNext would dispose its subscription within a call to OnNext. The built in AnonymousAsyncEnumerator checks whether it's already disposed, however, not every user code might handle canceled tokens that gracefully. Also, the continuation of MoveNext will not be scheduled anyway.

f();

if (!ctd.Token.IsCancellationRequested)
f();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code path won't trigger a dispose of the enumerator, unlike the case where MoveNext observes the cancellation and causes the returned Task to be in a canceled state.

@martinwoodward
Copy link
Member

Hi @danielcweber - while the team are looking into this one, would you mind signing the .NET Foundation Contribution License agreement? Shouldn't take long and is hopefully pretty painless. Many thanks!

@martinwoodward
Copy link
Member

Sorry - ignore me. I see you signed it already on June 8 2015

@danielcweber
Copy link
Collaborator Author

I fixed the enumerator-dispose-issue. Should be good now.

@clairernovotny
Copy link
Member

Hi @danielcweber: can you please verify the original issue against the https://github.com/Reactive-Extensions/Rx.NET/tree/ix-optimizations branch?

I've been making a lot of improvements there based on the CoreFX designs and fixed a number of issues related to this.

Thanks!

@danielcweber
Copy link
Collaborator Author

danielcweber commented Aug 1, 2016

I rebased it on ix-optimizations but cannot edit the target branch, thats why we have 9 commits here now. I could open a new pull request though.

@clairernovotny
Copy link
Member

Before rebasing it / including this fix, can we confirm that it's still needed? I made changes to the underlying AsyncIterator that has a wide effect and may mean it's not needed...so that's what I was looking to confirm?

@danielcweber
Copy link
Collaborator Author

danielcweber commented Aug 1, 2016

I would say the change still has a valid point, unless you check the state of the AsyncIterator after this line. An already cancelled token will immediately trigger Dispose, but MoveNextCore will still be executed right? I don't see anything throwing.

Note: I updated the link to the correct line.

@clairernovotny
Copy link
Member

clairernovotny commented Aug 1, 2016

That code looks like it's missing a few commits to optimization branch from this morning.

So the scenario here is that we are not calling/checking the cancellation status before calling MoveNextCore and thus we're potentially doing more work than necessary. Fair point and I can add a throwIfCancelled before the MoveNextCore call.

Would that solve this?

Note: I'm not against the PR, but I'm just trying to ensure we fix this issue, if it still exists, in the right location to cover all cases.

@danielcweber
Copy link
Collaborator Author

The point of my pull request is to avoid any throwing. The CancellationToken passed to MoveNext will be cancelled if the subscription on the observable (from ToObservable) is disposed. The consumer will not expect any calls to OnNext or OnError. However, if MoveNext fails, OnError will be called here.

@danielcweber
Copy link
Collaborator Author

danielcweber commented Aug 1, 2016

That last statement of mine is probably false, since the continuation will not be scheduled if ctd.Token is canceled. Still, having exceptions be thrown that could have been avoided is bad style.

@clairernovotny
Copy link
Member

Okay, so this fix is indeed highly specific to the IObservable impl? That's ok, thanks -- can you please rebase this back to master and we can get this included?

@clairernovotny
Copy link
Member

One other thing -- please include a test that verifies the desired behavior so we can ensure we don't break again.

Thanks

@danielcweber
Copy link
Collaborator Author

If a canceled token makes MoveNext return false instead of throwing a TaskCanceledException (as in your latest change) then we don't need the fix in this pull request at all. I'm just a little confused here - is returning false really the expected behaviour ?

@clairernovotny
Copy link
Member

clairernovotny commented Aug 1, 2016

@danielcweber good point....fixed the code to throw to ensure behavior is the same as if cancelled "in-flight". It just potentially short-circuits MoveNextCore.

So it appears your specific IObservable fix is still needed. If you could update it to be based on master and include a test, I'll get it merged in.

Thanks!

@danielcweber
Copy link
Collaborator Author

For ToObservable, getting false leads definitely to the right behavior, and there's no uncaught exception. Still there's the unnecessary call to MoveNext but that's ok I guess. Let's leave it as it is at the moment and see if there's other opinions. I'll rebase it back on master now.

@clairernovotny
Copy link
Member

@danielcweber thanks! Please just add a unit test for this behavior and I'll merge it in.

@clairernovotny
Copy link
Member

I see a test failure in the CI build:

Tests.AsyncTests.ToObservable_does_not_call_MoveNext_again_when_subscription_is_disposed()

@danielcweber
Copy link
Collaborator Author

I will investigate this. Test passed locally (of course, right?). Sorry for inconvenience.

…t with an already canceled CancellationToken. This would happen whenever an IObserver<T>.OnNext would dispose its subscription within a call to OnNext. The built in AnonymousAsyncEnumerator<T> checks whether it's already disposed, however, not every user code might handle canceled tokens that gracefully. Also, the continuation of MoveNext will not scheduled anyway.
… disposed and MoveNext is not called after subscription is disposed.
@danielcweber
Copy link
Collaborator Author

Tests are passing now, the failing build is probably due to the master also failing to build at the moment.

@clairernovotny
Copy link
Member

Yup, thanks, fixed the build script in master.

@clairernovotny clairernovotny merged commit 539bfa9 into dotnet:master Aug 3, 2016
@danielcweber danielcweber deleted the FixToObservableInIx branch August 5, 2016 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants