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 support for Response trailers for netstandard20 #18

Merged
merged 4 commits into from
Jul 19, 2023

Conversation

simonferquel
Copy link

When using the netstandard20 version, response trailers were previously ignored. This was causing issues with handling responses from a GRPC service.

Grpc-dotnet works with WinHttpHandler on recent Windows versions, and trailers are stored in the request generic property bag as shown here: https://github.com/grpc/grpc-dotnet/blob/456098bcfeb9c796c19ea023b3ab3703181f1089/src/Shared/TrailingHeadersHelpers.cs#L30

This PR uses the same way to store trailers when compiled for netstandard 2.0

Signed-off-by: Simon Ferquel [email protected]

When using the netstandard20 version, response trailers were
previously ignored. This was causing issues with handling
responses from a GRPC service.

Grpc-dotnet works with WinHttpHandler on recent Windows versions, and
trailers are stored in the request generic property bag as shown here:
https://github.com/grpc/grpc-dotnet/blob/456098bcfeb9c796c19ea023b3ab3703181f1089/src/Shared/TrailingHeadersHelpers.cs#L30

This PR uses the same way to store trailers when compiled for netstandard 2.0

Signed-off-by: Simon Ferquel <[email protected]>
@TalAloni
Copy link
Owner

Thanks, can you explain what the use-case is?
IIUC - this change is only useful for HTTP 2.0, and given that .NET 4.7.2 does not support ALPN, it is only useful for unencrypted HTTP 2.0 communication, which is not very common.
If one requires HTTP 2.0 and ALPN, this change is probably not going to help, right?

@simonferquel
Copy link
Author

This is a Unity use case, where we have some programs running grpc services on a named pipe - without encryption (on Windows platform). Those services can be consumed in MSBuild tasks, which run on .NET 4.7.2 within Visual Studio.
This change + the connect callback allows to connect to those services :) - otherwise grpc client complains that the response is missing some trailers.

@TalAloni
Copy link
Owner

that responseMessage.RequestMessage.Properties["__ResponseTrailers"] logic seems very use-case specific... isn't it?
not sure that's the right place, and besides, the 3.1 branch is experimental anyway

@tgjones
Copy link

tgjones commented Jul 12, 2023

that responseMessage.RequestMessage.Properties["__ResponseTrailers"] logic seems very use-case specific... isn't it?

@TalAloni it is the "official" place to put response trailing headers when running on .NET Framework: reference

On .NET Framework, trailers are added to HttpRequestMessage.Properties["__ResponseTrailers"]. Use with caution and only in well justified compatibility scenarios, since the property is obsolete

Any chance you could re-review this PR please, with that added context?


private async Task SendDataAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken)
}
class HttpTrailers : HttpHeaders
Copy link
Owner

@TalAloni TalAloni Jul 12, 2023

Choose a reason for hiding this comment

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

Should be a separate file and should only be defined for NETSTANDARD2_0

Copy link

Choose a reason for hiding this comment

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

Done 👍

@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Owner

Choose a reason for hiding this comment

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

You have removed the BOM from the beginning of the file, please revert this change

Copy link

Choose a reason for hiding this comment

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

Done 👍

@TalAloni TalAloni merged commit e9e7f75 into TalAloni:3.1 Jul 19, 2023
@tgjones
Copy link

tgjones commented Jul 20, 2023

@TalAloni thank you for merging! Please could you release a new version of the hidden 3.1.0.x package on NuGet?

@TalAloni
Copy link
Owner

TalAloni commented Jul 21, 2023

Done (v3.1.0.5)

@tgjones
Copy link

tgjones commented Jul 21, 2023

Thank you!

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.

3 participants