Repository navigation
fix(connect): the face knows the server's limit before the client's first call, and tells it its own - #117
Merged
Conversation
…irst call, and tells it its own Follow-ups m1 and m2 of review r77 on #110: - m1: the relay read the client at once, so a call opened before the server's first SETTINGS met the face's default (ENHANCE_YOUR_CALM) where the server would have refused it itself, retry-safe. The relay now reads the server's connection preface (its SETTINGS frame, bounded by read_timeout) before it reads the client. - m2: a server that advertises no SETTINGS_MAX_CONCURRENT_STREAMS (grpcio's default) has the face's DEFAULT_MAX_OPEN_CALLS added to its first SETTINGS, so a client that keeps to HTTP/2 queues its calls within what the face enforces. The client acknowledges that one frame as the server's, so the server sees the ACK it expects. 1100 calls on one grpc.aio channel are now all answered (1000 before, the rest reset). Tests: test_a_server_advertising_no_limit_is_given_the_faces, test_the_client_is_read_once_the_servers_limit_is_known and test_a_burst_past_the_limit_queues_and_loses_no_call fail on main; test_a_server_limit_reaches_the_client_untouched. The fake servers in the suite now open with SETTINGS, as a server's preface does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups m1 and m2 from review r77 of #110, which has merged. This is the whole change.
What changes
read_timeout) before it reads anything from the client.SETTINGS_MAX_CONCURRENT_STREAMS. In that case the face addsDEFAULT_MAX_OPEN_CALLS(1000) to the server's first SETTINGS frame, so a client that follows HTTP/2 queues its calls within what the face enforces. The client acknowledges that one frame as the server's, so the server sees the single ACK it expects.Classification: fix, in the face, which owns this behaviour.
Tests
These fail on main:
test_a_server_advertising_no_limit_is_given_the_facestest_the_client_is_read_once_the_servers_limit_is_known: the server's limit is 2 and arrives 0.3 s after three calls. The third call reaches the server untouched.test_a_burst_past_the_limit_queues_and_loses_no_call: 1100 out of 1100 answered.Also new:
test_a_server_limit_reaches_the_client_untouched.The suite's fake gRPC servers now open with SETTINGS, as a real server's preface does.
Checks
go vetand the Connect Go tests pass.relayperf, 200 MB: 1.59 s for 64 B frames and 0.11 s for 16 KiB frames.go test ./...fails onlyTestConnectOnlyServiceSyncs(from fix: a service whose faces are gRPC and Connect alone loads, syncs and starts #113). Buf's BSR rate-limits this machine ("resource_exhausted: too many requests"), so the failure is environmental and unrelated to this diff.🤖 Generated with Claude Code