Skip to content

Commit

Permalink
Use registration order to prioritize client Accept-Encoding (connectr…
Browse files Browse the repository at this point in the history
…pc#269)

We're currently randomizing the client's list of accepted encodings when
sending them to the server. This is bad, since the server uses the first
algorithm it recognizes: ideally the client would send them in
preference order.

This PR changes the client to treat the last registered algorithm as the
most preferred. To minimize divergence, we do the same for handlers.

Fixes connectrpc#259.
  • Loading branch information
akshayjshah authored Jun 6, 2022
1 parent 9b9d9e8 commit 11020f9
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 15 deletions.
8 changes: 6 additions & 2 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,11 @@ func NewClient[Req, Res any](httpClient HTTPClient, url string, options ...Clien
client.config = config
protocolClient, protocolErr := client.config.Protocol.NewClient(
&protocolClientParams{
CompressionName: config.RequestCompressionName,
CompressionPools: newReadOnlyCompressionPools(config.CompressionPools),
CompressionName: config.RequestCompressionName,
CompressionPools: newReadOnlyCompressionPools(
config.CompressionPools,
config.CompressionNames,
),
Codec: config.Codec,
Protobuf: config.protobuf(),
CompressMinBytes: config.CompressMinBytes,
Expand Down Expand Up @@ -173,6 +176,7 @@ type clientConfig struct {
CompressMinBytes int
Interceptor Interceptor
CompressionPools map[string]*compressionPool
CompressionNames []string
Codec Codec
RequestCompressionName string
BufferPool *bufferPool
Expand Down
15 changes: 10 additions & 5 deletions compression.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,19 @@ type readOnlyCompressionPools interface {
CommaSeparatedNames() string
}

func newReadOnlyCompressionPools(nameToPool map[string]*compressionPool) readOnlyCompressionPools {
knownNames := make([]string, 0, len(nameToPool))
for name := range nameToPool {
knownNames = append(knownNames, name)
func newReadOnlyCompressionPools(
nameToPool map[string]*compressionPool,
reversedNames []string,
) readOnlyCompressionPools {
// Client and handler configs keep compression names in registration order,
// but we want the last registered to be the most preferred.
names := make([]string, 0, len(reversedNames))
for i := len(reversedNames) - 1; i >= 0; i-- {
names = append(names, reversedNames[i])
}
return &namedCompressionPools{
nameToPool: nameToPool,
commaSeparatedNames: strings.Join(knownNames, ","),
commaSeparatedNames: strings.Join(names, ","),
}
}

Expand Down
6 changes: 5 additions & 1 deletion handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ func (h *Handler) ServeHTTP(responseWriter http.ResponseWriter, request *http.Re

type handlerConfig struct {
CompressionPools map[string]*compressionPool
CompressionNames []string
Codecs map[string]Codec
CompressMinBytes int
Interceptor Interceptor
Expand Down Expand Up @@ -319,7 +320,10 @@ func (c *handlerConfig) newProtocolHandlers(streamType StreamType) []protocolHan
}
handlers := make([]protocolHandler, 0, len(protocols))
codecs := newReadOnlyCodecs(c.Codecs)
compressors := newReadOnlyCompressionPools(c.CompressionPools)
compressors := newReadOnlyCompressionPools(
c.CompressionPools,
c.CompressionNames,
)
for _, protocol := range protocols {
handlers = append(handlers, protocol.NewHandler(&protocolHandlerParams{
Spec: c.newSpec(streamType),
Expand Down
18 changes: 11 additions & 7 deletions option.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ type ClientOption interface {

// WithAcceptCompression makes a compression algorithm available to a client.
// Clients ask servers to compress responses using any of the registered
// algorithms. It's safe to use this option liberally: servers will ignore any
// algorithms. The first registered algorithm is treated as the least
// preferred, and the last registered algorithm is the most preferred.
//
// It's safe to use this option liberally: servers will ignore any
// compression algorithms they don't support. To compress requests, pair this
// option with WithSendCompression.
//
Expand Down Expand Up @@ -251,18 +254,19 @@ type compressionOption struct {
}

func (o *compressionOption) applyToClient(config *clientConfig) {
o.apply(config.CompressionPools)
if o.Name == "" || o.CompressionPool == nil {
return
}
config.CompressionPools[o.Name] = o.CompressionPool
config.CompressionNames = append(config.CompressionNames, o.Name)
}

func (o *compressionOption) applyToHandler(config *handlerConfig) {
o.apply(config.CompressionPools)
}

func (o *compressionOption) apply(m map[string]*compressionPool) {
if o.Name == "" || o.CompressionPool == nil {
return
}
m[o.Name] = o.CompressionPool
config.CompressionPools[o.Name] = o.CompressionPool
config.CompressionNames = append(config.CompressionNames, o.Name)
}

type compressMinBytesOption struct {
Expand Down

0 comments on commit 11020f9

Please sign in to comment.