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

Remove System.Interactive.Async dependency #19059

Merged
merged 4 commits into from Jul 29, 2019

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented May 17, 2019

Addresses #18592

As discussed in the issue, this is a breaking change. I do not see anyway to resolve this problem without a breaking change.

I have copied across the properties and methods used from this assembly. Changes required to update source code will be small to none.

Keeping the current dependency is not a good solution:

  1. Anyone who needs to update to System.Interactive.Async 4.0 will not be able to use Grpc.Core because of the missing types.
  2. Anyone who uses Grpc.Core with .NET Core 3 and later will get a compiler error when IAsyncEnumerable is present in their source code:

image

// @jtattermusch @JunTaoLuo @davidfowl

@davidfowl
Copy link

@davidfowl davidfowl commented May 17, 2019

Should we take a dependency on the async interfaces package?

Loading

@JamesNK
Copy link
Member Author

@JamesNK JamesNK commented May 17, 2019

Do you mean https://www.nuget.org/packages/Microsoft.Bcl.AsyncInterfaces?

That package is only available in preview. Grpc.Core.Api needs to release a stable version and can't depend on Microsoft.Bcl.AsyncInterfaces until there is a stable version.

Loading

@davidfowl
Copy link

@davidfowl davidfowl commented May 17, 2019

That's fine, just keep it on the radar for non .NET Core 3.0 (though it's not a strict requirement)

Loading

@jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented May 19, 2019

@apolcyn any opinions on this? It's a big change with potential to break lot of users but I don't quite see any workarounds. More details about the issue are in #18592.

@jskeet what would be the impact of this change on the API client libraries?

@JamesNK can you provide some background on why System.Collections.Generic.IAsyncEnumerator is being removed from System.Interactive.Async? It seems like a change that is going to break a lot of people.

Loading

@jskeet
Copy link
Contributor

@jskeet jskeet commented May 19, 2019

If we do this, Grpc.Core will need a major version bump, and so will all our client libraries that have gone GA. That may still be the right option, but it's certainly painful. That said, we've got pain ahead for .NET Core 3 users as well.

What I think would be helpful at this point is detailed guidance from MS for library authors, giving the options and the impact on various users. (Apologies if that already exists.) We're not the only ones in this situation, I'm sure - and it would be good for everyone to be consistent as far as possible, and if the problem was understood consistently too.

Loading

@JamesNK
Copy link
Member Author

@JamesNK JamesNK commented May 19, 2019

@JamesNK can you provide some background on why System.Collections.Generic.IAsyncEnumerator is being removed from System.Interactive.Async? It seems like a change that is going to break a lot of people.

The status quo means there are duplicate namespace and assembly types. Resolving conflicts like this isn't easy. See point 2 - #19059 (comment)

What I think would be helpful at this point is detailed guidance from MS for library authors, giving the options and the impact on various users

We (Microsoft) don't own this library. I believe Reactive Extensions is a MS project that was taken over by the community.

@onovotny is there guidance that you could put on the https://github.com/dotnet/reactive for dealing with removing interfaces from System.Interactive.Async? Explain why they were removed and what the recommended migration path is. I think it would be standard guidance for dealing with a breaking change (update package reference to 4.0, resolving compile errors, increase your package's major version) but you might have thought about it more and have some extra advice.

Loading

@clairernovotny
Copy link

@clairernovotny clairernovotny commented May 19, 2019

Ix Async 4 will be a breaking change from 3.x out of necessity. The shape of the interface, as defined by C# 8, has changed. The BCL is also providing it via Microsoft.Bcl.AsyncInterfaces and .NET Standard 2.1.

There's no way we can have both the old and new interfaces in the 4.0 version, but it is a major version increment to delineate the breakage.

Loading

@jskeet
Copy link
Contributor

@jskeet jskeet commented May 20, 2019

It's the fact that C# 8 has forced this change that makes me think Microsoft should provide guidance here. It's not like Ix has decided to do this out of the blue. There may well be other folks with other slightly different interfaces who want to do the same thing as well.

I think it would make sense to move from Ix to Microsoft.Bcl.AsyncInterfaces as a dependency rather than have the interface in Grpc.Core being entirely separate from the rest of the world.
(A major version bump for Grpc.Core would also be a good time to reevaluate the target frameworks, e.g. potentially dropping netstandard1.5 support.)

Loading

@JamesNK
Copy link
Member Author

@JamesNK JamesNK commented May 20, 2019

I think it would make sense to move from Ix to Microsoft.Bcl.AsyncInterfaces as a dependency rather than have the interface in Grpc.Core being entirely separate from the rest of the world.

What benefit does IAsyncStreamReader<T> gain from implementing IAsyncEnumerator<T>? As far as I know, people generally don't use the enumerator, but the enumerable. I added an extension method to use the stream reader with async foreach via an extension method here - #19060

Loading

@jskeet
Copy link
Contributor

@jskeet jskeet commented May 20, 2019

What benefit does IAsyncStreamReader<T> gain from implementing IAsyncEnumerator<T>? As far as I know, people generally don't use the enumerator, but the enumerable. I added an extension method to use the stream reader with async foreach via an extension method here - #19060

It's definitely useful to have the extension method. I thought there were methods in Rx that allowed filtering, projection etc based on an IAsyncEnumerator<T> as well, but I could be wrong about that.

It does feel odd not to implement the new interface though. Admittedly requiring the dependency means we're blocked on that going GA, but it feels like that's a relatively small price to pay.

One other point: do we need IAsyncStreamReader<T> at all in reality? If we're creating a new major version, maybe we should remove it entirely in favour of IAsyncEnumerator<T>. At that point, anyone writing code which wants to implement it (and using C# 8) could use yield return from an iterator block, making it much easier to implement. This would at least make test code easier to write, for mocks.

Loading

@JamesNK
Copy link
Member Author

@JamesNK JamesNK commented May 20, 2019

One other point: do we need IAsyncStreamReader<T> at all in reality? If we're creating a new major version, maybe we should remove it entirely in favour of IAsyncEnumerator<T>.

Using the enumerator instead of the stream reader would involve changing gRPC method signatures in codegen, and that means Grpc.Tools would need additional options. That is more trouble than it is worth.

Loading

@jskeet
Copy link
Contributor

@jskeet jskeet commented May 20, 2019

Hmm. I agree that extra options is messy. I'd personally like to see an adapter from IAsyncEnumerator<T> to IAsyncStreamReader<T>, at least if we do take a dependency on Microsoft.Bcl.AsyncInterfaces. (It's probably not a sufficient justification on its own to take a dependency on that.)

Loading

@JamesNK
Copy link
Member Author

@JamesNK JamesNK commented May 20, 2019

gRPC would probably want to take a dependency on Microsoft.Bcl.AsyncInterfaces in netstandard20 to allow the extension method here to be available - #19060

IMO I don't see much value in implementing the async enumerator interface on stream reader, but there isn't much disadvantage either (other than having to wait until 3.0 is final before making these changes).

Loading

@jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented May 22, 2019

A few thoughts

  • looks like this is going to be a very painful process. Currently versions of all the C-core based languages are in sync so changing that is going to affect our release process.
  • we haven't made a breaking change for any of the languages since GA release a few years back, so suddenly bumping the major version for C# is going to make gRPC C# an oddity (and it's going to be confusing to the users because suddenly c# will be at completely different version numbers that e.g. the github releases on the grpc/grpc repo)
  • if we're really bumping the major version of C# to 2.x.x, I think I'd want to use that opportunity to -re-review the existing API and potentially make a few more API adjustments (and that process is going to take a while).
  • based on the previous conversation, we'd probably want to add a dependency on Microsoft.Bcl.AsyncInterfaces, but that package is currently in preview so we're blocked by that.

Loading

@mgravell
Copy link
Contributor

@mgravell mgravell commented Jun 14, 2019

If I can add some thoughts: first I acknowledge that nobody is underplaying the problem here. There's a wealth of good input above.

But: this is a major blocker, and delaying only adds pain. By being tied into the RX implementation, that means nothing can change until core 3 (for the RX major). Which means a number of other things then cannot even remotely work at core 3 time, because the dev work can't even begin until this is resolved. For example, right now I'm looking at API-reshaping dynamic proxies that work with patterns like IAsyncEnumerable-T: I can't touch it because of the conflict prevents everything completely.

It isn't my call to make, but I'd urge people to think sooner rather than later. It is just adding pain. And yes, it is a hard break. That ship sailed already.

Loading

@jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Jun 18, 2019

If I can add some thoughts: first I acknowledge that nobody is underplaying the problem here. There's a wealth of good input above.

But: this is a major blocker, and delaying only adds pain. By being tied into the RX implementation, that means nothing can change until core 3 (for the RX major). Which means a number of other things then cannot even remotely work at core 3 time, because the dev work can't even begin until this is resolved. For example, right now I'm looking at API-reshaping dynamic proxies that work with patterns like IAsyncEnumerable-T: I can't touch it because of the conflict prevents everything completely.

It isn't my call to make, but I'd urge people to think sooner rather than later. It is just adding pain. And yes, it is a hard break. That ship sailed already.

Thanks for the input @mgravell. We are currently trying to figure out what potentially a gRPC C# major version bump would mean for the rest of gRPC (currently all the languages are at the same version). As you can imagine this is not an easy decision to make, but we are aware of the urgency of the problem. Stay tuned.

Loading

@terrajobst
Copy link

@terrajobst terrajobst commented Jul 4, 2019

From reading this thread it sounds like the problem is that the existing System.Interactive version that you're depending on doesn't play nicely with .NET Core because both define IAsyncEnumerable<T>.

The Microsoft.Bcl.AsyncInterfaces NuGet package currently isn't marked as stable. Presume we'd give you a stable version immediately, is there any reason why you wouldn't replace IX with the Microsoft.Bcl.AsyncInterfaces?

Loading

@terrajobst
Copy link

@terrajobst terrajobst commented Jul 4, 2019

@jskeet

What I think would be helpful at this point is detailed guidance from MS for library authors, giving the options and the impact on various users.

People should upgrade to the latest version of IX that has the type forwarder for .NET Standard 2.1-based platforms so that you don't have duplicated types.

Loading

@jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Jul 16, 2019

@terrajobst the IAsyncEnumerable from Microsoft.Bcl.AsyncInterfaces has different methods than the IAsyncEnumerable we're currently using. The change proposed by this PR changes is so that we provide source-level backward compatibility (most users would just need to recompile).

If we actually changed MoveNext() to MoveNextAsync(), most users that are using streaming RPCs would need to manually change their code to remain compatible (and in some codebases there would be MANY occurrences to replace), which seems to be a much worse outcome.

So, the answer is no, Microsoft.Bcl.AsyncInterfaces becoming stable wouldn't help us. It would actually harm us if it happened sooner than changes from this PR are merged and released.

https://github.com/dotnet/corefx/blob/bb0516a4f8d0c4d84eff2ba2a3ad0a4764ece9d2/src/Microsoft.Bcl.AsyncInterfaces/ref/Microsoft.Bcl.AsyncInterfaces.cs#L24

Loading

/// <returns>
/// Task containing the result of the operation: true if the reader was successfully advanced
/// to the next element; false if the reader has passed the end of the sequence.</returns>
Task<bool> MoveNext(CancellationToken cancellationToken);

Choose a reason for hiding this comment

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

Is the goal to make this non-source breaking?

Loading

Copy link
Member Author

@JamesNK JamesNK Jul 17, 2019

Choose a reason for hiding this comment

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

Yeah

Loading

Choose a reason for hiding this comment

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

my source got broken here, would it make sence to do something like
CancellationToken cancellationToken = CancellationToken.None ?

Loading

Copy link
Contributor

@jtattermusch jtattermusch Oct 1, 2019

Choose a reason for hiding this comment

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

No, there's an extension method that provides the parameterless MoveNext():
https://github.com/grpc/grpc/pull/19059/files#diff-fa87174ba85fa18aefdd2f6f0c173750R39

you need to make sure that extension is visible from your code and you should be fine. (using Grpc.Core; should be enough?)

Loading

Choose a reason for hiding this comment

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

@jtattermusch thanks for your answer, i'm using v 2.23.
have using Grpc.Core;,
changed to while (await responseStream.MoveNext(CancellationToken.None)) to make it work

Loading

Copy link
Contributor

@jtattermusch jtattermusch left a comment

LGTM (as per grpc/proposal#154 which is going to be approved.)

Loading

@jtattermusch jtattermusch merged commit ede5a8e into grpc:master Jul 29, 2019
45 checks passed
Loading
Copy link
Contributor

@lidizheng lidizheng left a comment

LGTM. Doing sanity review for the import to ensure every PR has LGTMs from two Googler.

Loading

@@ -0,0 +1,50 @@
#region Copyright notice and license

// Copyright 2015 gRPC authors.
Copy link
Contributor

@lidizheng lidizheng Aug 2, 2019

Choose a reason for hiding this comment

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

s/2015/2019

Loading

@lock lock bot locked as resolved and limited conversation to collaborators Jan 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants