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

Add net462 build to Grpc.Net.Client with WinHttpHandler default #2220

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

stephentoub
Copy link
Contributor

No description provided.

@JamesNK JamesNK merged commit 6a4bca7 into grpc:master Aug 2, 2023
2 checks passed
@stephentoub stephentoub deleted the net462winhttphandler branch August 2, 2023 04:42
@jtattermusch
Copy link
Contributor

Can you please provide a description for the PR? I'd be curious what was the motivation behind adding net462 target.

@stephentoub
Copy link
Contributor Author

stephentoub commented Aug 2, 2023

Can you please provide a description for the PR? I'd be curious what was the motivation behind adding net462 target.

Sorry, yes, I should have provided more explanation.

The motivation is entirely this:

#if NET462
return new WinHttpHandler();

Today when someone uses this package in a .NET Framework application, it doesn't "just work"; they're required to realize there's a problem, add a nuget reference to System.Net.Http.WinHttpHandler, and pass in a WinHttpHandler instance as part of the channel options. And if the grpc client is an implementation detail in some other library being consumed, and that library hasn't either done the work itself to light-up with WinHttpHandler or hasn't exposed a configuration option to let the caller configure it, that library is then inherently broken when used with .NET Framework.

This change means that when the .NET Framework asset is resolved, it'll by default "just work". I discussed with @JamesNK making that happen dynamically in the netstandard2.0 asset, but that would require any consumer resolving the netstandard2.0 asset also pulling along the WinHttpHandler reference, and so this route was preferred.

@JamesNK
Copy link
Member

JamesNK commented Aug 2, 2023

var channel = GrpcChannel.ForAddress("https://localhost");

Before this used to error on .NET Framework and say that a handler must be provided. Now it configures itself with WinHttpHandler.

@jtattermusch
Copy link
Contributor

jtattermusch commented Aug 2, 2023

CC @jskeet

This is possibly quite significant for the client libraries.

@jskeet
Copy link

jskeet commented Aug 3, 2023

This could be a decidedly mixed blessing for us in the Google Cloud client libraries. We default to attempting to use Grpc.Net.Client, but if constructing a channel fails, we fall back on Grpc.Core (via reflection). So at the moment, anyone on .NET Framework ends up using Grpc.Core, but it works.

With this change in place, I believe we'll be in a better-and-worse position:

  • Users on a suitable version of Windows where Grpc.Net.Client works, e.g. a recent Windows 11 build, will start using Grpc.Net.Client by default instead of Grpc.Core. That's good - it's the direction of travel we want.
  • Users on a version of Windows where WinHttpHandler doesn't support gRPC fully (doesn't handle trailers etc) will end up with failures when they try to use the client, which is too late for us to fall back to Grpc.Core.

Does that sounds like I've understood this PR correctly?

To my mind, the ideal behavior would be "on .NET 4.6.2, use WinHttpHandler if it looks like we're on a suitable version of Windows, but continue to fail eagerly if WinHttpHandler won't work fully." Is that feasible?

@JamesNK
Copy link
Member

JamesNK commented Aug 3, 2023

if constructing a channel fails, we fall back on Grpc.Core (via reflection). So at the moment, anyone on .NET Framework ends up using Grpc.Core, but it works.

In the current version of Grpc.Net.Client, creating a channel in .NET Framework is guaranteed to throw an error. To perverse that behavior you could check if the app is .NET Framework and skip directly to using Grpc.Core.

I created an issue for adding a Windows version check when WinHttpHandler is used - #2226

@jtattermusch
Copy link
Contributor

Note that this change has made it to 2.56.0-pre1 release I just made:
https://github.com/grpc/grpc-dotnet/releases/tag/v2.56.0-pre1

I think I'll hold the stable release until we verify that this won't cause issues in the client libraries.
It seems to me that #2220 should only be released together with the fix for #2226 (to avoid having weird behavior of #2220 without having #2226).
@jskeet does that sound reasonable?

@jskeet
Copy link

jskeet commented Aug 3, 2023

Yes, I think having the two together would make sense. If my additional note for #2226 could be implemented as well, that would be even better, of course.

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