Skip to content

Commit 4ecf543

Browse files
fix(connect): every opening refusal is a valid server preface that names what happened
Review r121 of #118: - B1: the GOAWAY for a server that sent no SETTINGS was the face's first frame to the client; RFC 9113 3.4 makes a server's first frame SETTINGS, and grpcio discarded the reason ("Expected SETTINGS frame as the first frame"). Every refusal of an opening is now an empty SETTINGS, then the GOAWAY. A real grpcio client logs "Got goaway [2] ... Debug Text: the gRPC server sent no SETTINGS within 0.5 s". - m1: the channel's receive bound is clamped to 2**31 - 1 (gRPC's channel arguments are C ints), so a bound at gRPC's largest starts again. - m2: the reason says what happened: no SETTINGS within N s, closed before its SETTINGS, a first frame of N bytes past the 16384 allowed, or a first frame that is not SETTINGS. - m3: the 16 KiB first-frame cap is tested in both directions; a client's oversized first frame gets SETTINGS + GOAWAY(FRAME_SIZE_ERROR). - m4: only the server's direction scans SETTINGS; the client's skips them as before (scan_bench: 14 GB/s, main 15 GB/s). - m5: a first frame that is not a SETTINGS frame on stream 0 without ACK is refused, from either peer, with GOAWAY(PROTOCOL_ERROR) naming it; the client's never reaches the server. Tests (each fails on 43edffd): test_a_server_that_sends_no_settings_gets_the_client_a_goaway (silent, closed, oversized, not-settings), test_a_grpc_client_hears_why_the_server_never_answered (a grpcio client in its own process), test_a_client_that_sends_no_settings_is_told_why, test_a_client_whose_first_frame_is_not_its_settings_is_refused (oversized, headers, settings-ack), test_a_bound_at_grpcs_largest_starts, and the client direction in test_the_servers_limit_is_found_anywhere_in_its_settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 43edffd commit 4ecf543

2 files changed

Lines changed: 198 additions & 27 deletions

File tree

‎templates/connect/connect_gen.py‎

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,14 @@
9696
_END_STREAM_FLAG, _ACK_FLAG, _END_HEADERS_FLAG = 0x1, 0x1, 0x4
9797
_MAX_CONCURRENT_STREAMS = 0x3
9898
_PUSH_PROMISE = 0x5
99-
_PROTOCOL_ERROR, _INTERNAL_ERROR, _CANCEL, _ENHANCE_YOUR_CALM = 0x1, 0x2, 0x8, 0xB
100-
# The largest frame a peer may send before it has the other's SETTINGS (RFC 9113 4.2).
99+
_PROTOCOL_ERROR, _INTERNAL_ERROR, _FRAME_SIZE_ERROR = 0x1, 0x2, 0x6
100+
_CANCEL, _ENHANCE_YOUR_CALM = 0x8, 0xB
101+
# The largest frame a client may send before it has the server's SETTINGS (RFC 9113
102+
# 4.2); the face holds the server's first frame to it too, though the client's SETTINGS
103+
# could allow more (a SETTINGS frame that size would carry over 2,700 entries).
101104
_INITIAL_MAX_FRAME = 16384
105+
# gRPC's channel arguments are C ints.
106+
_GRPC_MAX_INT = 2**31 - 1
102107
# Calls one relayed connection may hold open when the gRPC server advertises no
103108
# SETTINGS_MAX_CONCURRENT_STREAMS (grpcio advertises none unless the service sets
104109
# grpc.max_concurrent_streams, as the scaffold's server does). The face advertises it to
@@ -294,11 +299,12 @@ class _AnswerBounds:
294299
them as they are, ``json`` for one that converts them, checked before converting.
295300
``received`` is the channel's own receive bound: one byte past both, so the face
296301
checks every bound itself, and the number grpcio names when it refuses past it is
297-
one no service's client has by chance with the same text."""
302+
one no service's client has by chance with the same text. A bound at gRPC's largest
303+
(2**31 - 1, its "unlimited") keeps the channel there."""
298304

299305
def __init__(self, proto: int, json: int):
300306
self.proto, self.json = proto, json
301-
self.received = max(proto, json) + 1
307+
self.received = min(max(proto, json) + 1, _GRPC_MAX_INT)
302308

303309
def of(self, codec: _Codec) -> tuple[int, str]:
304310
if codec.name == "proto":
@@ -344,13 +350,15 @@ class _Frames:
344350
where frames end in the data, each with the bytes ``on_frame`` returned, to be written
345351
right after that frame in this direction. The payload passed is, for a SETTINGS
346352
frame, its last SETTINGS_MAX_CONCURRENT_STREAMS entry if it has one (the frame is
347-
scanned whole as it streams, holding one entry), for a GOAWAY its first
353+
scanned whole as it streams, holding one entry, when ``scan_settings``: the server's
354+
direction only, since only the server's limit matters), for a GOAWAY its first
348355
``_CONTROL_PAYLOAD`` bytes, and empty otherwise. A
349356
``_ProtocolError`` from ``on_frame`` stops the feed; it is kept in ``error`` and the
350357
ends before the offending frame are returned."""
351358

352-
def __init__(self, on_frame):
359+
def __init__(self, on_frame, *, scan_settings: bool = False):
353360
self._on_frame = on_frame
361+
self._scan_settings_frames = scan_settings
354362
self._header = bytearray()
355363
self._frame = None # (type, flags, stream) while its payload is read
356364
self._after = b"" # what on_frame returned for it
@@ -429,7 +437,7 @@ def _start(self, high, low, kind, flags, stream):
429437
if kind in (_HEADERS, _PUSH_PROMISE, _CONTINUATION):
430438
self._in_block = not flags & _END_HEADERS_FLAG
431439
self._keep = min(length, _CONTROL_PAYLOAD) if kind == _GOAWAY else 0
432-
self._scan = length if kind == _SETTINGS else 0
440+
self._scan = length if kind == _SETTINGS and self._scan_settings_frames else 0
433441
self._skip = length - self._keep - self._scan
434442
if not (self._keep or self._scan):
435443
self._emit()
@@ -450,17 +458,43 @@ def _emit(self):
450458
self._after = self._on_frame(*self._frame, payload) or b""
451459

452460

461+
class _FrameSizeError(_ProtocolError):
462+
"""A first frame past ``_INITIAL_MAX_FRAME``."""
463+
464+
453465
async def _first_frame(reader) -> bytes:
454-
"""A peer's first frame, whole: its SETTINGS (RFC 9113 section 3.4), within the frame
455-
size any peer may send before it has the other's."""
466+
"""A peer's first frame, whole, which RFC 9113 section 3.4 makes its SETTINGS: a
467+
SETTINGS frame on stream 0, not an acknowledgement, within ``_INITIAL_MAX_FRAME``."""
456468
header = await reader.readexactly(_FRAME.size)
457-
high, low, *_ = _FRAME.unpack(header)
469+
high, low, kind, flags, stream = _FRAME.unpack(header)
458470
length = high << 16 | low
459471
if length > _INITIAL_MAX_FRAME:
460-
raise _ProtocolError(f"a first frame of {length} bytes")
472+
raise _FrameSizeError(
473+
f"a first frame of {length} bytes, past the {_INITIAL_MAX_FRAME} "
474+
"allowed before SETTINGS"
475+
)
476+
if kind != _SETTINGS or flags & _ACK_FLAG or stream:
477+
raise _ProtocolError(
478+
f"a first frame of type {kind} (flags {flags}, stream {stream}), not SETTINGS"
479+
)
461480
return header + await reader.readexactly(length)
462481

463482

483+
def _opening_refusal(exc: BaseException, peer: str, timeout: float) -> str:
484+
"""Why a peer's opening failed, in words an operator can act on."""
485+
if isinstance(exc, TimeoutError):
486+
return f"the {peer} sent no SETTINGS within {timeout:g} s"
487+
if isinstance(exc, _ProtocolError):
488+
return f"the {peer} sent {exc}"
489+
return f"the {peer} closed the connection before its SETTINGS"
490+
491+
492+
def _refuse_opening(writer, code: int, reason: str) -> None:
493+
"""Tell the client why its connection ends, as a server's first frames must: SETTINGS
494+
first (RFC 9113 section 3.4: a client discards anything else), then the GOAWAY."""
495+
writer.write(_h2_frame(_SETTINGS, 0, 0) + _goaway(0, code, reason))
496+
497+
464498
def _with_max_streams(frame: bytes, limit: int) -> bytes:
465499
"""A server SETTINGS frame that advertises no SETTINGS_MAX_CONCURRENT_STREAMS, with
466500
``limit`` added: the client then holds its calls within what the face enforces. The
@@ -560,7 +594,7 @@ def from_server(kind, flags, stream, payload):
560594
calls.difference_update({call for call in calls if call > last_stream})
561595
return None
562596

563-
client, server = _Frames(from_client), _Frames(from_server)
597+
client, server = _Frames(from_client), _Frames(from_server, scan_settings=True)
564598

565599
def tell_client(frame):
566600
nonlocal told
@@ -648,7 +682,11 @@ async def idle():
648682
async with asyncio.timeout(self.read_timeout):
649683
opening = await _first_frame(reader)
650684
except opening_errors as exc:
651-
_log.info("connect face: the client sent no SETTINGS: %r", exc)
685+
reason = _opening_refusal(exc, "client", self.read_timeout)
686+
_log.info("connect face: %s", reason)
687+
if isinstance(exc, (TimeoutError, _ProtocolError)):
688+
code = _FRAME_SIZE_ERROR if isinstance(exc, _FrameSizeError) else _PROTOCOL_ERROR
689+
_refuse_opening(writer, code, reason)
652690
upstream_writer.close()
653691
return
654692
client.feed(opening)
@@ -660,9 +698,9 @@ async def idle():
660698
async with asyncio.timeout(self.read_timeout):
661699
preface = await _first_frame(upstream_reader)
662700
except opening_errors as exc:
663-
reason = f"the gRPC server sent no SETTINGS within {self.read_timeout:g} s"
664-
_log.warning("connect face: %s: %r", reason, exc)
665-
writer.write(_goaway(0, _INTERNAL_ERROR, reason))
701+
reason = _opening_refusal(exc, "gRPC server", self.read_timeout)
702+
_log.warning("connect face: %s", reason)
703+
_refuse_opening(writer, _INTERNAL_ERROR, reason)
666704
upstream_writer.close()
667705
return
668706
server.feed(preface)

‎testdata/connect/test_connect_gen.py‎

Lines changed: 144 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import asyncio
55
import base64
66
import collections
7+
import os
8+
import sys
79
import json
810
import struct
911

@@ -595,8 +597,9 @@ async def test_connect_json_ignores_unknown_fields(face):
595597
assert r.json() == {"text": "hi|maya", "count": 1}
596598

597599

598-
async def test_a_silent_grpc_connection_is_closed_after_the_idle_bound():
599-
# An upstream that accepts and never answers: only the face's own bound ends it.
600+
async def test_a_client_that_sends_no_settings_is_told_why():
601+
# A client that sends the preface's magic and then nothing: the face's read bound
602+
# ends it, and the client is told why, after the SETTINGS a server's first frame is.
600603
upstream = await asyncio.start_server(lambda r, w: None, "127.0.0.1", 0)
601604
listener = await connect_gen.serve(
602605
0,
@@ -609,7 +612,10 @@ async def test_a_silent_grpc_connection_is_closed_after_the_idle_bound():
609612
reader, writer = await asyncio.open_connection("127.0.0.1", listener.port)
610613
writer.write(b"PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n")
611614
await writer.drain()
612-
assert await asyncio.wait_for(reader.read(), 3) == b""
615+
sent = read_frames(await asyncio.wait_for(reader.read(), 3))
616+
assert [f[:3] for f in sent] == [(0x4, 0, 0), (0x7, 0, 0)]
617+
reason = b"the client sent no SETTINGS within 0.5 s"
618+
assert sent[1][3] == struct.pack(">II", 0, 0x1) + reason
613619
writer.close()
614620
finally:
615621
await listener.stop()
@@ -938,15 +944,134 @@ async def accept(reader, writer):
938944
upstream.close()
939945

940946

941-
async def test_a_server_that_sends_no_settings_gets_the_client_a_goaway(silent_upstream):
947+
def _refusal(got):
948+
"""The face's refusal of a connection: (GOAWAY code, reason), after an empty SETTINGS."""
949+
frames = read_frames(got)
950+
assert [f[:3] for f in frames] == [(0x4, 0, 0), (0x7, 0, 0)], frames
951+
assert frames[0][3] == b""
952+
last, code = struct.unpack_from(">II", frames[1][3])
953+
assert last == 0
954+
return code, frames[1][3][8:].decode()
955+
956+
957+
@pytest.mark.parametrize(
958+
"preface, reason",
959+
[
960+
(None, "the gRPC server sent no SETTINGS within 0.5 s"),
961+
(b"", "the gRPC server closed the connection before its SETTINGS"),
962+
(
963+
h2_frame(0x4, 0, 0, b"\0" * 16386),
964+
"the gRPC server sent a first frame of 16386 bytes, past the 16384 allowed "
965+
"before SETTINGS",
966+
),
967+
(
968+
h2_frame(0x6, 0, 0, b"\0" * 8),
969+
"the gRPC server sent a first frame of type 6 (flags 0, stream 0), not SETTINGS",
970+
),
971+
],
972+
ids=["silent", "closed", "oversized", "not-settings"],
973+
)
974+
async def test_a_server_that_sends_no_settings_gets_the_client_a_goaway(preface, reason):
975+
# Whatever went wrong, the client hears why, after an empty SETTINGS (RFC 9113 3.4:
976+
# a server's first frame; a client discards anything else).
977+
async def accept(reader, writer):
978+
if preface is not None:
979+
writer.write(preface)
980+
await writer.drain()
981+
if preface == b"":
982+
writer.close()
983+
return
984+
while await reader.read(65536):
985+
pass
986+
987+
upstream = await asyncio.start_server(accept, "127.0.0.1", 0)
988+
listener = await connect_gen.serve(
989+
0, upstream.sockets[0].getsockname()[1], host="127.0.0.1", read_timeout=0.5
990+
)
991+
try:
992+
closed, got = await exchange(listener, PREFACE + SETTINGS, 2)
993+
assert closed
994+
assert _refusal(got) == (0x2, reason)
995+
finally:
996+
await listener.stop()
997+
upstream.close()
998+
999+
1000+
_GRPC_CLIENT = """
1001+
import sys, grpc
1002+
channel = grpc.insecure_channel(sys.argv[1])
1003+
try:
1004+
channel.unary_unary("/echo.v1.Echo/Say")(b"", timeout=2)
1005+
except grpc.RpcError as error:
1006+
print(error.code())
1007+
"""
1008+
1009+
1010+
async def test_a_grpc_client_hears_why_the_server_never_answered():
1011+
# A real gRPC client, in its own process so its transport log can be read: it parses
1012+
# the face's refusal (a GOAWAY first would be "Expected SETTINGS frame") and logs
1013+
# its reason.
1014+
async def accept(reader, writer):
1015+
while await reader.read(65536):
1016+
pass
1017+
1018+
upstream = await asyncio.start_server(accept, "127.0.0.1", 0)
1019+
listener = await connect_gen.serve(
1020+
0, upstream.sockets[0].getsockname()[1], host="127.0.0.1", read_timeout=0.5
1021+
)
1022+
try:
1023+
client = await asyncio.create_subprocess_exec(
1024+
sys.executable,
1025+
"-c",
1026+
_GRPC_CLIENT,
1027+
f"127.0.0.1:{listener.port}",
1028+
stdout=asyncio.subprocess.PIPE,
1029+
stderr=asyncio.subprocess.PIPE,
1030+
env={**os.environ, "GRPC_VERBOSITY": "info"},
1031+
)
1032+
out, err = await asyncio.wait_for(client.communicate(), 30)
1033+
log = err.decode()
1034+
assert "Expected SETTINGS frame" not in log
1035+
assert "Debug Text: the gRPC server sent no SETTINGS within 0.5 s" in log, log[-2000:]
1036+
finally:
1037+
await listener.stop()
1038+
upstream.close()
1039+
1040+
1041+
@pytest.mark.parametrize(
1042+
"opening, code, reason",
1043+
[
1044+
(
1045+
h2_frame(0x4, 0, 0, b"\0" * 16386),
1046+
0x6,
1047+
"the client sent a first frame of 16386 bytes, past the 16384 allowed before SETTINGS",
1048+
),
1049+
(
1050+
h2_frame(0x1, 0x4, 1, b"\x82"),
1051+
0x1,
1052+
"the client sent a first frame of type 1 (flags 4, stream 1), not SETTINGS",
1053+
),
1054+
(
1055+
h2_frame(0x4, 0x1, 0),
1056+
0x1,
1057+
"the client sent a first frame of type 4 (flags 1, stream 0), not SETTINGS",
1058+
),
1059+
],
1060+
ids=["oversized", "headers", "settings-ack"],
1061+
)
1062+
async def test_a_client_whose_first_frame_is_not_its_settings_is_refused(
1063+
silent_upstream, opening, code, reason
1064+
):
9421065
face, state = silent_upstream
943-
state["preface"] = b"" # a server that never sends its SETTINGS
944-
closed, got = await exchange(await face(read_timeout=0.5), PREFACE + SETTINGS, 2)
1066+
closed, got = await exchange(await face(), PREFACE + opening, 2)
9451067
assert closed
946-
((kind, _, _, payload),) = read_frames(got)
947-
last, code = struct.unpack_from(">II", payload)
948-
assert (kind, last, code) == (0x7, 0, 0x2)
949-
assert payload[8:] == b"the gRPC server sent no SETTINGS within 0.5 s"
1068+
assert _refusal(got) == (code, reason)
1069+
assert bytes(state["received"]) == PREFACE # the frame never reached the server
1070+
1071+
1072+
async def test_a_bound_at_grpcs_largest_starts():
1073+
listener = await connect_gen.serve(0, 1, host="127.0.0.1", max_response_bytes=2**31 - 1)
1074+
await listener.stop()
9501075

9511076

9521077
async def test_the_servers_limit_is_found_anywhere_in_its_settings():
@@ -958,11 +1083,19 @@ async def test_the_servers_limit_is_found_anywhere_in_its_settings():
9581083
assert (0x3, connect_gen.DEFAULT_MAX_OPEN_CALLS) not in _settings(to_client[0])
9591084

9601085
seen = []
961-
frames = connect_gen._Frames(lambda kind, flags, stream, payload: seen.append(payload))
1086+
frames = connect_gen._Frames(
1087+
lambda kind, flags, stream, payload: seen.append(payload), scan_settings=True
1088+
)
9621089
for at in range(len(server_settings)):
9631090
frames.feed(server_settings[at : at + 1])
9641091
assert seen == [struct.pack(">HI", 0x3, 7)] and frames.at_boundary
9651092

1093+
# The client's direction only skips them: its SETTINGS cost nothing to read.
1094+
seen.clear()
1095+
frames = connect_gen._Frames(lambda kind, flags, stream, payload: seen.append(payload))
1096+
frames.feed(server_settings)
1097+
assert seen == [b""] and frames.at_boundary
1098+
9661099

9671100
async def test_a_server_advertising_no_limit_is_given_the_faces():
9681101
# grpcio advertises no SETTINGS_MAX_CONCURRENT_STREAMS: the client is told the face's

0 commit comments

Comments
 (0)