Skip to content

Conversation

@iblancasa
Copy link

@iblancasa iblancasa commented Nov 20, 2025

  • Add grpc.WithAcceptedCompressionNames so a client can explicitly cap the grpc-accept-encoding header to a vetted subset of registered compressors
  • Ensure the new value is propagated to the proper code while still appending per-call legacy
    compressors when needed

Updates #2786.

RELEASE NOTES:

  • client: Add grpc.WithAcceptedCompressionNames so callers can restrict the grpc-accept-encoding header advertised by a ClientConn.

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 88.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.24%. Comparing base (50c6321) to head (6470258).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
rpc_util.go 86.66% 3 Missing and 3 partials ⚠️
stream.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8718      +/-   ##
==========================================
+ Coverage   83.21%   83.24%   +0.02%     
==========================================
  Files         419      419              
  Lines       32427    32475      +48     
==========================================
+ Hits        26985    27034      +49     
- Misses       4054     4055       +1     
+ Partials     1388     1386       -2     
Files with missing lines Coverage Δ
experimental/experimental.go 33.33% <100.00%> (+33.33%) ⬆️
internal/transport/http2_client.go 92.92% <100.00%> (+0.02%) ⬆️
internal/transport/transport.go 90.86% <ø> (ø)
server.go 81.96% <100.00%> (-0.49%) ⬇️
stream.go 81.88% <87.50%> (+0.04%) ⬆️
rpc_util.go 83.64% <86.66%> (+0.21%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eshitachandwani
Copy link
Member

Hey @iblancasa , thank you for the PR! Before we make the changes , we should probably discuss and decide among the different suggestions Doug had in the issue :

Another consideration: we should probably allow per-client/server registries of codecs which either overrides the global registry, or adds to it and provides a separate client/server option to disable the global registry.

cc : @dfawley

@iblancasa
Copy link
Author

iblancasa commented Nov 21, 2025

Thanks for your quick response @eshitachandwani. I would be fine with the current solution or @dfawley suggestion. I'm willing to contribute that approach if it is what is being decided.

@dfawley
Copy link
Member

dfawley commented Nov 21, 2025

Since the headers are sent per-call, this kind of a setting really wants to be a CallOption and not a DialOption. (They can be "converted", too, by using WithDefaultCallOptions).

I think the basic idea here is fine and doesn't overlap with my suggestion of a separate registry of compressors per-channel. This would be saying "for this call, only allow compressors x/y/z for responses".

There's also the question of what to do if the response comes back with a compressor the client technically supports, but isn't advertising for that call. Should it be cancelled or allowed?

Also I would prefer to add this as experimental until we can determine it is fine as-is and doesn't need different behavior or a different API. These days we are doing that by adding something into the experimental package, which makes things a little circuitous but helps people know they are using something that we reserve the right to change. (See also the recent issue I filed about this topic #8683 if interested, mainly the second comment.)

@iblancasa
Copy link
Author

@dfawley thanks for your feedback. I tried to address it in my last push. Regarding:

There's also the question of what to do if the response comes back with a compressor the client technically supports, but isn't advertising for that call. Should it be cancelled or allowed?

I feel it makes sense to not accept it. I implemented on that way. We can change it in the future if we receive some feedback. What do you think?

@eyewill085-code
Copy link

दरबारी

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.

4 participants