serve streaming Connect RPCs natively - #271
Open
hugowetterberg wants to merge 2 commits into
Open
Conversation
A service that never serves Twirp can implement connect-go's own handler interface rather than the plain protobuf one, and that is what lets it declare streaming methods: protoc-gen-elephant-rpc and protoc-gen-twirp both fail generation on a stream. Five things in the library were wrong for a streaming handler and invisible for a unary one. Message size limits are per message on a Connect mount. APIServer no longer wraps a Connect subtree's request body in an http.MaxBytesReader, which counts the bytes read over the life of the request and so would have ended a client or bidirectional stream once its cumulative traffic passed 8 MiB. ServiceOptions gains MaxMessageBytes, defaulting to DefaultMaxBodyBytes, and MaxSendMessageBytes, which is off unless set: a send limit restricts responses that connect-go and the Twirp mount both serve unbounded, and connect-go checks it against the compressed wire size, so defaulting it on would refuse the same call for one caller and serve it to another. Every non-Connect mount keeps the stream-level limit, and the 413 for an oversized declared Content-Length stays on every path. rpc.DrainInterceptor ends streaming handlers with unavailable when the server starts shutting down. http.Server.Shutdown does not cancel request contexts, so a streaming handler previously learned nothing about a deploy, held shutdown open until the timeout, and then had its connection closed underneath it. APIServer starts the drain before Shutdown, and the wait is now APIServerShutdownTimeout rather than a hard-coded ten seconds. Streams are observed in rpc_stream_duration_seconds and counted while open in rpc_streams_active. rpc_duration_seconds is unary only: a subscription's lifetime in a histogram whose top bucket is thirty seconds lands in +Inf and drags every quantile computed over the service with it. No existing series was renamed or relabelled. rpc.ContextWithTokenExpiry ends a stream with unauthenticated when the caller's token expires. It is a helper a handler calls rather than a default, since whether a subscription may outlive the authorization that opened it differs from stream to stream. The request-scoped log metadata map is mutex-guarded and GetLogMetadata returns a copy. A streaming handler that runs a producer goroutine alongside the one writing to the stream is the normal shape, and two of them calling SetLogMetadata is a concurrent map write. The fixture gains stream.proto with one method per stream type, generated by a second buf run with only protoc-gen-go and protoc-gen-connect-go, which is what a native service's generation looks like. buf.yaml turns on the STANDARD lint rules, grandfathering rpc/errormeta.proto: elephantine.rpc.ErrorMeta is the error-detail type name on the wire in every error body in the fleet, so its package can never move. docs/connect.md now covers the native shape and streaming alongside the dual-stack material it already had.
connect.md stated the layout rule as the whole story, which would have told a service author that an existing service can only become native by moving to the versioned layout — and so by changing its proto package, which is in its procedure path. The three shapes are named, the layout is described as the default it is, and rpc.Shapes is shown overriding it in both directions, with the reason it has to: a service adopting streaming should not have to break every caller's path to get there. Also says what does change for a consumer when a service goes native — the plain-interface client constructor gives way to connect-go's *connect.Client — and that a library break is deliberately the only kind in that move.
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.
Summary
A service that never serves Twirp can implement connect-go's own handler interface rather than the plain protobuf one, and that is what lets it declare streaming methods:
protoc-gen-elephant-rpcandprotoc-gen-twirpboth fail generation on a stream. Five things in the library were wrong for a streaming handler and invisible for a unary one, so none of them was caught by the dual-stack tests.Message size limits are per message on a Connect mount.
APIServerno longer wraps a Connect subtree's request body in anhttp.MaxBytesReader, which counts the bytes read over the life of the request — and for a client or bidirectional stream the request body is the stream, so such a stream would have died once its cumulative traffic passed 8 MiB.ServiceOptionsgainsMaxMessageBytes, defaulting toDefaultMaxBodyBytes, andMaxSendMessageBytes, which is off unless set. Every non-Connect mount keeps the stream-level limit, and the413for an oversized declaredContent-Lengthstays on every path.rpc.DrainInterceptorends streaming handlers withunavailableat shutdown.http.Server.Shutdowndoes not cancel request contexts, so a streaming handler previously learned nothing about a deploy, held shutdown open until the timeout, and then had its connection closed underneath it — a truncated stream with no code, and no cleanup.APIServerstarts the drain beforeShutdown, and the wait is nowAPIServerShutdownTimeoutrather than a hard-coded ten seconds.Streams have their own metric series.
rpc_stream_duration_secondsandrpc_streams_active;rpc_duration_secondsis unary only, since a subscription's lifetime in a histogram whose top bucket is thirty seconds lands in+Infand drags every quantile computed over the service with it. No existing series was renamed or relabelled.rpc.ContextWithTokenExpiryends a stream withunauthenticatedwhen the caller's token expires. A helper a handler calls rather than a default, since whether a subscription may outlive the authorization that opened it differs from stream to stream.The request-scoped log metadata map is mutex-guarded, and
GetLogMetadatareturns a copy. A streaming handler that runs a producer goroutine alongside the one writing to the stream is the normal shape, and two of them callingSetLogMetadatais a concurrent map write.Why
MaxSendMessageBytesdefaults to offA read limit protects the service from its callers and replaces one that was already there. A send limit protects the caller from the service, and nothing has ever bounded what an elephant service may answer with — connect-go leaves it off, and the Twirp mount of a dual-stack service has no response limit at all. Defaulting it on would refuse responses the same service already serves, and refuse them unpredictably: connect-go checks the limit against the bytes on the wire, so a response is measured compressed when the caller accepts compression, and the identical call passes for one caller and is refused for another.
Testing
The fixture gains
stream.protowith one method per stream type, generated by a second buf run with onlyprotoc-gen-goandprotoc-gen-connect-go— which is what a native service's generation looks like. Before this the streaming interceptor wrappers, written deliberately rather than withconnect.UnaryInterceptorFunc, were covered by nothing at all.Also
buf.yamlturns on the STANDARD lint rules, grandfatheringrpc/errormeta.proto:elephantine.rpc.ErrorMetais the error-detail type name on the wire in every error body in the fleet, so its package can never move. The internal fixture service was renamedTesttoTestServiceto satisfySERVICE_SUFFIX.docs/connect.mdnow covers the native shape and streaming alongside the dual-stack material it already had.