fix(grpc/http): review follow-ups — mask drift, body caps, flow-control, HEAD, compression API#109
Merged
Merged
Conversation
Contributor
CoverageTotal lines: 81.31% → 81.04% (-0.26 pp)
❌ Regression in touched files (> 1.0 pp drop)
Add |
…ol, HEAD, compression API Review of the merged gRPC branch (#104) surfaced 7 confirmed bugs plus layering/cleanup debt. Fixes, no behaviour change to passing paths: Correctness: - protocol→mask: single http_protocol_registration_mask() used by both addXHandler registration and the worker-side transit rebuild, so a gRPC-only server keeps the HTTP2 bit on worker threads (h2c no longer silently refused). - H2/H3 streaming request bodies: restore the cumulative max_body_size ceiling that a live-only cap had dropped; waived per-stream only for gRPC (unbounded by design). In-flight cap still bounds memory. - H3 buffered→streaming upgrade: track body_precredited so spliced prefix bytes aren't flow-control-credited twice. - HEAD: response->is_head stamped at dispatch; send() drops chunks so no DATA follows the header block (RFC 9110 §9.3.2) on every transport. - grpc-timeout: clamp to UINT64_MAX instead of uint64 overflow wrap. - worker sink: check STREAM_HEADERS post result before building the chunk wire; replace the 100ms blocking nanosleep retry with a hidden one-shot timer + per-thread FIFO so the worker thread never blocks. - reactor_pool_exec: escape the done-wait loop when the reactor reaches DONE (latent shutdown-hang guard). Layering / performance: - Classify gRPC once: http_request_classify_protocols() stamps grpc_mode + body policy flags at headers-complete; transports read neutral flags and no longer include grpc.h or re-parse content-type per readMessage. - Compression API: setGrpcEncoding('gzip') declared before the first message (per-call, matching grpc-go/java/c++); drop the per-message writeMessage($msg, $compress) arg — a compressed frame with no declared encoding can no longer be expressed. - Event-driven credit backpressure: reactor wakes the parked producer via stream_credit_wake instead of 2–32ms timer polling; same for the H3 static throttle hysteresis wake. Cleanup: - response_wire_discard() replaces three copy-pasted discard blocks. - async_coroutine_sleep_ms() shared helper (was 4 near-identical copies). - http_protocol_pick_handler() in the H3 coroutine entry. - delete the dead http3_build_listener_local wrapper.
EdmondDantes
force-pushed
the
fix/grpc-review-followups
branch
from
86c7537 to
04aa3ad
Compare
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
1 participant
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-up fixes from a code review of the merged gRPC branch (#104). Seven confirmed correctness bugs plus layering/perf/cleanup debt. No behaviour change to already-passing paths; clean build.
Correctness
http_protocol_registration_mask()is now the single source for the protocol→mask rule, used by bothaddXHandlerregistration and the worker-side transit rebuild. Previously the rebuild dropped theHTTP2bit for a gRPC-only server, so worker threads silently closed plaintext h2c connections.max_body_sizeceiling was restored for H2/H3 streaming bodies (a live-queued-only cap had replaced it). Waived per-stream only for gRPC (streaming RPCs are unbounded by design); the in-flight window still bounds memory.body_precreditedso the spliced prefix isn't window-credited twice.response->is_headstamped at dispatch;send()drops chunks so no DATA follows the header block (RFC 9110 §9.3.2), across all transports.UINT64_MAXinstead of wrapping to a bogus small deadline.nanosleepretry is replaced by a hidden one-shot timer + per-thread FIFO so the worker thread never blocks.Layering / performance
http_request_classify_protocols()stampsgrpc_mode+ body-policy flags at headers-complete; transports read neutral flags, no longer#include grpc.hor re-parse content-type perreadMessage().setGrpcEncoding('gzip')declared before the first message (per-call, matching grpc-go/java/c++/python); the per-messagewriteMessage($msg, $compress)arg is removed, so a compressed frame with no declared encoding can no longer be expressed.stream_credit_wakeinstead of 2–32ms timer polling; same hysteresis wake for the H3 static throttle.Cleanup
response_wire_discard()replaces three copy-pasted discard blocks.async_coroutine_sleep_ms()shared helper (was 4 near-identical copies).http_protocol_pick_handler()in the H3 coroutine entry.http3_build_listener_localwrapper.Tests
gRPC 15/15, h2+h3 80/80 pass.
008-grpc-compressionupdated to the newsetGrpcEncodingAPI.core/055–057(pool graceful-shutdown) is present onmainindependent of this PR — proven with a clean-tree repro. Tracked separately.