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

Ix: Do not dispose the enumerator while enumerating in the ToObservable operator. #915

Merged
merged 9 commits into from Nov 20, 2019

Conversation

quinmars
Copy link
Contributor

@quinmars quinmars commented Jun 5, 2019

As pointed out by @danielcweber the current implementation of ToObservable may violate the contract of IAsyncEnumerator, because it is possible to call DisposeAsync during an on going MoveNextAsync call.

#423 (comment)

Copy link
Contributor Author

@quinmars quinmars left a comment

Choose a reason for hiding this comment

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

I'm using now the original logic without catching Rx pipeline exceptions. There are two places, however, where the enumerator could throw without being catched atm.

@danielcweber
Copy link
Collaborator

Review by @bartdesmet still needed? His earlier review has been adressed by @quinmars if I see this correctly.

@quinmars
Copy link
Contributor Author

Somehow, github could not link the commit to @bartholinie. Does any one know how to make that work?

@danielcweber
Copy link
Collaborator

"Jens Chr authored and quinmars committed 12 hours ago". Commit info is preserved, isn't it?

@danielcweber
Copy link
Collaborator

danielcweber commented Nov 20, 2019

The requested change was about the recursion and has been adressed, right? Would merge after checks pass.

@danielcweber danielcweber merged commit 46d757b into dotnet:master Nov 20, 2019
@quinmars
Copy link
Contributor Author

Right the commit info is preserve, but @bartholinie is not listed in the contributors list.

@danielcweber
Copy link
Collaborator

@onovotny @bartdesmet Can we have a Linq.Async / Ix.Async preview that would contain this fix? Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants