fix(grpc): enforce types with Dialyzer and fix related violations#551
Closed
smartinio wants to merge 2 commits into
Closed
fix(grpc): enforce types with Dialyzer and fix related violations#551smartinio wants to merge 2 commits into
smartinio wants to merge 2 commits into
Conversation
Expose helpers for parsing and encoding embedded trailer frames.
Run Dialyzer in CI for grpc. Update channel and stream types to match values returned by client code, and fix the Gun embedded trailer branch instead of widening types around it.
Contributor
|
Thanks, but I don't think we should support Dialyzer anymore. The proper fix for us is to run Elixir 1.20 warnings as errors and to remove all explicit @SPEC annotations. |
1 task
Contributor
Author
|
@polvalente Fair enough. Opened one for that in #552 |
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.
Problem
Dialyzer reports
GRPC.Stub.connect/2as returning a value outside its own spec.connect/2returns aGRPC.Channelstruct, butGRPC.Channel.t()is stricter than the channel states built by the client connection code. Downstream projects that dialyze calls toconnect/2see that as a broken public type.GRPC.Client.Stream.t()has the same issue for fields that start asniland are filled later.Turning Dialyzer on also catches a Gun server stream trailer bug. Embedded gRPC trailers are handled like Gun HTTP/2 trailers, so callers get raw trailer bytes as a stream item instead of decoded trailers at the end of the stream. Mint has the same embedded-trailer bug, but Dialyzer does not flag it because
codec.decode/2acceptsany.Root cause
The
ChannelandStreamstructs support incrementally populating their values, but the public types describe only fully populated state. Type specs exist, but without a CI step checking them, this drift is not caught when implementation changes.The
GRPC.Client.Connectionwarnings above started in PR #527, where load balancing started picking fullChannelstructs per request. That would have been caught by Dialyzer in CI.For client adapter response streams, embedded gRPC trailers and adapter trailer messages are different events. Embedded gRPC trailers are carried in DATA frame payloads. Gun and Mint both have adapter-level trailer handling for HTTP/2 trailers, but embedded trailers need to be decoded from the DATA payload first. Gun sends binary trailer payloads through the HTTP/2 trailer branch. Mint sends the same binary payloads through
codec.decode/2as if they were response messages.Fix
Run Dialyzer for
grpcin a separate CI job:dialyxirtogrpcdeps/_buildartifacts and Dialyzer PLTs, based on elixir-mint's CI setupFix existing violations to make Dialyzer pass:
GRPC.Channel.t()andGRPC.Client.Stream.t()to match values returned by client codeReview attention
This PR also exposes
grpc_corehelpers for embedded trailer payload/frame handling, to avoid low-level gRPC internals like the0b1000_0000trailers flag leaking into adapter code. However,grpcconsumesgrpc_corethrough the package dependency, so the Gun and Mint adapters cannot use the new core helper until the next publish ofgrpc_core. TODOs mark that temporary split.Test plan
:nofinand:findatagrpc_coretests for parsing and encoding embedded trailers