Skip to content

Commit

Permalink
Remove WithMaxReadBytes
Browse files Browse the repository at this point in the history
The more I look at it, the more convinced I am that this option is a bad
idea. It's very unclear what it's trying to accomplish, and there are
many better options:

* Limiting heap usage? Use the upcoming soft memory limit APIs
  (golang/go#48409).
* Limiting network I/O? Use `http.MaxBytesReader` and set a per-stream
  limit.
* Banning "large messages"? Be clear what you mean, and use
  `unsafe.SizeOf` or `proto.Size` in an interceptor.

Basically, the behavior here (and in grpc-go) is an incoherent middle
ground between Go runtime settings, HTTP-level settings, and a vague "no
large messages" policy.

I'm doubly sure we should delete this because we've decided not to
expose the metrics to track how close users are to the configured limit
:)
  • Loading branch information
akshayjshah committed Apr 7, 2022
1 parent bfc288f commit 04b42bd
Show file tree
Hide file tree
Showing 10 changed files with 14 additions and 125 deletions.
2 changes: 0 additions & 2 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ func NewClient[Req, Res any](
CompressionPools: newReadOnlyCompressionPools(config.CompressionPools),
Codec: config.Codec,
Protobuf: config.protobuf(),
MaxResponseBytes: config.MaxResponseBytes,
CompressMinBytes: config.CompressMinBytes,
HTTPClient: httpClient,
URL: url,
Expand Down Expand Up @@ -166,7 +165,6 @@ func (c *Client[Req, Res]) newStream(ctx context.Context, streamType StreamType)
type clientConfiguration struct {
Protocol protocol
Procedure string
MaxResponseBytes int64
CompressMinBytes int
Interceptor Interceptor
CompressionPools map[string]compressionPool
Expand Down
2 changes: 0 additions & 2 deletions handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ func (h *Handler) failNegotiation(w http.ResponseWriter, code int) {
type handlerConfiguration struct {
CompressionPools map[string]compressionPool
Codecs map[string]Codec
MaxRequestBytes int64
CompressMinBytes int
Interceptor Interceptor
Procedure string
Expand Down Expand Up @@ -293,7 +292,6 @@ func (c *handlerConfiguration) newProtocolHandlers(streamType StreamType) []prot
Spec: c.newSpecification(streamType),
Codecs: codecs,
CompressionPools: compressors,
MaxRequestBytes: c.MaxRequestBytes,
CompressMinBytes: c.CompressMinBytes,
}))
}
Expand Down
3 changes: 1 addition & 2 deletions handler_example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ func Example_handler() {
// (for example, net/http's StripPrefix).
mux := http.NewServeMux()
mux.Handle(pingv1connect.NewPingServiceHandler(
&ExamplePingServer{}, // our business logic
connect.WithReadMaxBytes(1024*1024), // limit request size
&ExamplePingServer{}, // our business logic
))
// You can serve gRPC's health and server reflection APIs using
// github.com/bufbuild/connect-grpchealth-go and github.com/bufbuild/connect-grpcreflect-go.
Expand Down
66 changes: 0 additions & 66 deletions handler_test.go

This file was deleted.

25 changes: 0 additions & 25 deletions option.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,19 +231,6 @@ func WithProtoJSONCodec() Option {
return WithCodec(&protoJSONCodec{})
}

// WithReadMaxBytes limits the performance impact of pathologically large
// messages sent by the other party. For handlers, WithReadMaxBytes limits the size
// of message that the client can send. For clients, WithReadMaxBytes limits the
// size of message that the server can respond with. Limits are applied before
// decompression and apply to each Protobuf message, not to the stream as a
// whole.
//
// Setting WithReadMaxBytes to zero allows any message size. Both clients and
// handlers default to allowing any request size.
func WithReadMaxBytes(n int64) Option {
return &readMaxBytesOption{n}
}

type clientOptionsOption struct {
options []ClientOption
}
Expand Down Expand Up @@ -363,18 +350,6 @@ func (o *optionsOption) applyToHandler(config *handlerConfiguration) {
}
}

type readMaxBytesOption struct {
Max int64
}

func (o *readMaxBytesOption) applyToClient(config *clientConfiguration) {
config.MaxResponseBytes = o.Max
}

func (o *readMaxBytesOption) applyToHandler(config *handlerConfiguration) {
config.MaxRequestBytes = o.Max
}

type requestCompressionOption struct {
Name string
}
Expand Down
2 changes: 0 additions & 2 deletions protocol.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ type protocolHandlerParams struct {
Spec Specification
Codecs readOnlyCodecs
CompressionPools readOnlyCompressionPools
MaxRequestBytes int64
CompressMinBytes int
}

Expand Down Expand Up @@ -91,7 +90,6 @@ type protocolClientParams struct {
CompressionName string
CompressionPools readOnlyCompressionPools
Codec Codec
MaxResponseBytes int64
CompressMinBytes int
HTTPClient HTTPClient
URL string
Expand Down
20 changes: 7 additions & 13 deletions protocol_grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ func (g *protocolGRPC) NewHandler(params *protocolHandlerParams) protocolHandler
web: g.web,
codecs: params.Codecs,
compressionPools: params.CompressionPools,
maxRequestBytes: params.MaxRequestBytes,
minCompressBytes: params.CompressMinBytes,
accept: acceptPostValue(g.web, params.Codecs),
}
Expand All @@ -57,7 +56,6 @@ func (g *protocolGRPC) NewClient(params *protocolClientParams) (protocolClient,
compressionPools: params.CompressionPools,
codec: params.Codec,
protobuf: params.Protobuf,
maxResponseBytes: params.MaxResponseBytes,
minCompressBytes: params.CompressMinBytes,
httpClient: params.HTTPClient,
procedureURL: params.URL,
Expand All @@ -69,7 +67,6 @@ type grpcHandler struct {
web bool
codecs readOnlyCodecs
compressionPools readOnlyCompressionPools
maxRequestBytes int64
minCompressBytes int
accept string
}
Expand Down Expand Up @@ -171,7 +168,6 @@ func (g *grpcHandler) NewStream(
g.web,
responseWriter,
request,
g.maxRequestBytes,
g.minCompressBytes,
clientCodec,
g.codecs.Protobuf(), // for errors
Expand Down Expand Up @@ -212,7 +208,6 @@ type grpcClient struct {
compressionPools readOnlyCompressionPools
codec Codec
protobuf Codec
maxResponseBytes int64
minCompressBytes int
httpClient HTTPClient
procedureURL string
Expand Down Expand Up @@ -255,14 +250,13 @@ func (g *grpcClient) NewStream(
// the response stream.
pipeReader, pipeWriter := io.Pipe()
duplex := &duplexClientStream{
ctx: ctx,
httpClient: g.httpClient,
url: g.procedureURL,
spec: spec,
maxReadBytes: g.maxResponseBytes,
codec: g.codec,
protobuf: g.protobuf,
writer: pipeWriter,
ctx: ctx,
httpClient: g.httpClient,
url: g.procedureURL,
spec: spec,
codec: g.codec,
protobuf: g.protobuf,
writer: pipeWriter,
marshaler: marshaler{
writer: pipeWriter,
compressionPool: g.compressionPools.Get(g.compressionName),
Expand Down
14 changes: 6 additions & 8 deletions protocol_grpc_client_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,12 @@ func (cr *clientReceiver) Trailer() http.Header { return cr.stream.ResponseTrail
// request/response code. Since this is the most complex code in connect, it has
// many more comments than usual.
type duplexClientStream struct {
ctx context.Context
httpClient HTTPClient
url string
spec Specification
maxReadBytes int64
codec Codec
protobuf Codec // for errors
ctx context.Context
httpClient HTTPClient
url string
spec Specification
codec Codec
protobuf Codec // for errors

// send: guarded by prepareOnce because we can't initialize this state until
// the first call to Send.
Expand Down Expand Up @@ -334,7 +333,6 @@ func (cs *duplexClientStream) makeRequest(prepared chan struct{}) {
mergeHeaders(cs.responseHeader, res.Header)
cs.unmarshaler = unmarshaler{
reader: res.Body,
max: cs.maxReadBytes,
codec: cs.codec,
compressionPool: cs.compressionPools.Get(compression),
web: cs.web,
Expand Down
2 changes: 0 additions & 2 deletions protocol_grpc_handler_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ func newHandlerStream(
web bool,
responseWriter http.ResponseWriter,
request *http.Request,
maxReadBytes int64,
compressMinBytes int,
codec Codec,
protobuf Codec, // for errors
Expand All @@ -53,7 +52,6 @@ func newHandlerStream(
unmarshaler: unmarshaler{
web: web,
reader: request.Body,
max: maxReadBytes,
compressionPool: requestCompressionPools,
codec: codec,
},
Expand Down
3 changes: 0 additions & 3 deletions protocol_grpc_lpm.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ func (m *marshaler) writeGRPCPrefix(compressed, trailer bool, size int) *Error {

type unmarshaler struct {
reader io.Reader
max int64
codec Codec
compressionPool compressionPool

Expand Down Expand Up @@ -169,8 +168,6 @@ func (u *unmarshaler) Unmarshal(message any) (retErr *Error) {
size := int(binary.BigEndian.Uint32(prefixes[1:5]))
if size < 0 {
return errorf(CodeInvalidArgument, "message size %d overflowed uint32", size)
} else if u.max > 0 && int64(size) > u.max {
return errorf(CodeInvalidArgument, "message size %d is larger than configured max %d", size, u.max)
}
// OPT: easy opportunity to pool buffers and grab the underlying byte slice
raw := make([]byte, size)
Expand Down

0 comments on commit 04b42bd

Please sign in to comment.