Unenforced discard limits

[emcfarlane]

An internal discard limit is applied to limit discarding bytes from a connection on close. It's currently at 4MiB: https://github.com/connectrpc/connect-go/blob/734ea9430653fa7956c53a06c3f139ac41d9407a/protocol.go#L43

However when connect receives a large message payload it discards the entire message to read the max size and report an error :

• https://github.com/connectrpc/connect-go/blob/734ea9430653fa7956c53a06c3f139ac41d9407a/envelope.go#L253-L257
• https://github.com/connectrpc/connect-go/blob/734ea9430653fa7956c53a06c3f139ac41d9407a/protocol_connect.go#L1087-L1091

This avoids the discard limit and will read up to the max message size (max uint32, and potentially more in connect unary?). This also can read much more than the connect.WithReadMaxBytes limited, although it won't keep the total payload in memory.

To Reproduce

To showcase I wrote a quick test to time a large payload with a server that has a 2MiB limit. Each iteration takes around 4.5s locally. Altering the bounds to error and not discard reduce this to less than 1s (presumably the time to marshal and start sending the large payload).

func TestShowDiscard(t *testing.T) {
	mux := http.NewServeMux()
	mux.Handle(
		pingv1connect.NewPingServiceHandler(
			&ExamplePingServer{},
			connect.WithReadMaxBytes(2*1024*1024), // 2 MiB
		),
	)
	server := httptest.NewServer(mux)
	t.Cleanup(server.Close)

	httpClient := server.Client()
	httpTransport, ok := httpClient.Transport.(*http.Transport)
	assert.True(t, ok)
	httpTransport.DisableCompression = true

	clients := []struct {
		name string
		opts []connect.ClientOption
	}{{
		name: "connect",
		opts: []connect.ClientOption{},
	}, {
		name: "grpc",
		opts: []connect.ClientOption{
			connect.WithGRPC(),
		},
	}, {
		name: "grpcweb",
		opts: []connect.ClientOption{
			connect.WithGRPCWeb(),
		},
	}}

	bigPayload := strings.Repeat("a", math.MaxUint32-10)
	msg := &pingv1.PingRequest{Text: bigPayload}
	size := proto.Size(msg)
	t.Log("size", size)
	if size > math.MaxUint32 {
		t.Fatal("message too big")
	}
	for _, client := range clients {
		now := time.Now()
		t.Log("start", client.name, now)
		client := pingv1connect.NewPingServiceClient(
			httpClient,
			server.URL,
			connect.WithClientOptions(client.opts...),
		)
		if _, err := client.Ping(
			context.Background(), connect.NewRequest(&pingv1.PingRequest{Text: bigPayload}),
		); err != nil {
			t.Log(err) // resource_exhausted: message size 4294967291 is larger than configured max 2097152
		}
		t.Log("duration", time.Since(now))
	}
}

Open questions

1. Should the discard limits be applied to message discarding?
2. Does the number of bytes read affect the discard limit?

Additional Info Opened #606 to explore a solution, but needs clarification on how discard limits should be calculated.

[emcfarlane]

Connect docs for WithReadMaxBytes mention http.MaxBytesHandler as an alternative.

Handlers may also use http.MaxBytesHandler to limit the total size of the HTTP request stream (rather than the per-message size). Connect handles http.MaxBytesError specially, so clients still receive errors with the appropriate error code and informative messages.

The current behaviour is not interchangeable to only being applied per message or per handler. http.MaxBytesHandler will try to close the connection and does not allow any more reads to progress once the limit is hit. Whilst connect.WithReadMaxBytes restricts the buffer size of a single message but doesn't stop more reads or close the connection.

[jhump]

Whilst connect.WithReadMaxBytes restricts the buffer size of a single message but doesn't stop more reads or close the connection.

This is for client and bidi stream endpoints, too? I thought that if the handler detected that this limit was exceeded that it results in a "resource exhausted" error to the client. That suggests that the RPC is immediately failed. But it sounds like what you're saying is that doesn't happen automatically -- it would require the handler to propagate the error that was returned from Receive. And if the error is ignored, the handler could keep going and ultimately return a different error (or even no error). Is that right?

Seems like possibly a bug.

Unclear what should happen on the client side though -- if it received a message that was too large and wanted to fail the RPC, the best it can do is to cancel, which really isn't the same. Also, "exhausting the response stream" typically means calling stream.Receive until it returns an error, but this sounds like this makes that not true -- you could get an error, and iff it's "resource exhausted", the caller could (should?) keep receiving. That feels kinda icky.

[emcfarlane]

I thought that if the handler detected that this limit was exceeded that it results in a "resource exhausted" error to the client.

Sorry I was unclear. The http.MaxBytesHandler will stop any Read() calls to the body guaranteeing that Read() can only get X bytes. The connect.WithReadMaxBytes will keep allowing Read() calls but does return an error and should (?) close the connection on the returned error. Was raising this to figure out how to factor the discard limit into the read limit. Should we allow more Read() calls per message then WithReadMaxBytes limits?

Had not even thought about Receive() error being ignored. Maybe need to look at that too.