fix(client): prune a channel when its adapter connection process dies - #570
Open
pfitz wants to merge 1 commit into
Open
fix(client): prune a channel when its adapter connection process dies#570pfitz wants to merge 1 commit into
pfitz wants to merge 1 commit into
Conversation
Adapter connection processes are linked to `GRPC.Client.Connection`, but
the `{:EXIT, ...}` handler only logged a warning. The channel stayed
`{:connected, ch}` in `real_channels` and stayed in the load balancer's
list, so `pick_channel/2` kept handing out a channel whose `conn_pid` was
dead. Callers then fail with `:noproc` from `GenServer.call/3` -- the
adapters only guard against `conn_pid: nil`, and a dead pid is not nil.
`lb_mod.update/2` has a single call site, reachable only from the
re-resolution path, so recovery depended entirely on a resolver tick.
Only `Resolver.DNS` implements the optional `init/2` that starts that
timer, so for `ipv4:`, `ipv6:` and `unix:` targets no tick ever comes and
the dead channel was served for the lifetime of the connection.
Handle the exit instead: mark the channel failed, rebalance the load
balancer so it stops being picked, and -- when it was the last connected
channel -- clear `established?` and restart the establish loop, which is
otherwise a no-op while `established?` is true.
Emit `[:grpc, :client, :connection, :channel_pruned]` so the prune is
observable, and generalise the "no healthy channels" warning, which is
now reachable outside re-resolution.
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.
Fixes #569.
Problem
Adapter connection processes are linked to
GRPC.Client.Connection, but the{:EXIT, …}handler only logged a warning. The channel stayed{:connected, ch}inreal_channelsand stayed in the load balancer's list, sopick_channel/2kept handing out a channel whoseconn_pidwas dead.Callers then fail with
:noproc— the adapters guard only againstconn_pid: nil(mint.ex:84), and a dead pid is not nil, so it falls through toGenServer.call(dead_pid, …).lb_mod.update/2has a single call site, reachable only from the re-resolution path, so recovery depended entirely on a resolver tick. OnlyResolver.DNSimplements the optionalinit/2that starts that timer, so foripv4:/ipv6:/unix:targets no tick is ever coming and the dead channel was served for the lifetime of the connection.Change
In the exit handler, when the exiting pid backs one of our channels:
{:failed, reason}.rebalance_after_reconcile/2, so the balancer stops picking it.established?and schedule:retry_establish.Step 3 matters: pruning alone would swap "serves a dead channel" for "serves nothing" on literal-address targets, since nothing else would ever rebuild it.
:retry_establishis a no-op whileestablished?is true, so that flag has to be cleared for the retry to run.Exits from pids that don't back a channel keep the previous behaviour (log and ignore) — including the resolver worker, whose gating in the clause above is untouched.
Two smaller things that follow from the change:
[:grpc, :client, :connection, :channel_pruned](measurement:remaining, metadata adds:reasonand:address), documented in the moduledoc alongside the existing events. It makes the prune observable, and it's what lets the tests synchronise without polling — the exit signal originates in the dying process, so it isn't ordered against anything the test process sends."No healthy channels available after re-resolution"→"No healthy channels available", since that branch is now reachable outside re-resolution.Tests
New
GRPC.Test.ProcessClientAdapterbacks each channel with a real linked process recorded asconn_pid, the way Gun and Mint do —GRPC.Test.ClientAdapterreturns the channel untouched, so there was no process to kill.Three tests in a new
"adapter connection process exits"block::connectedevent, then a live pid{:EXIT, …}prunes nothingThe first two fail on
masterand pass with the change; the third passes either way and is there as a regression guard.mix testingrpc/: 370 passed, 2 skipped.mix format --check-formattedclean.Verified end to end
Against the reproduction in #569,
ipv4:target — before, step 3 returned the dead pid and step 5 still did after a fullresolve_interval. After:The
dns://target recovers the same way, now in ~100ms rather than up to 30s.Not addressed here
When several addresses are connected and one dies, the failed address is pruned but not re-dialled until the next reconcile — so on a literal-address target it stays out. That felt like a separate change (it needs per-address retry state), and the current behaviour is still strictly better than serving a dead channel. Happy to fold it in if you'd rather.
This also doesn't touch the missing failure-feedback callback on
GRPC.Client.LoadBalancing, which is the deeper gap described in #569 — that's a design call for you rather than something to slip into a bug fix.