fix: stop leaking client supervisor children on failed and closed connections#554
fix: stop leaking client supervisor children on failed and closed connections#554mvanhorn wants to merge 2 commits into
Conversation
|
@mvanhorn Some tests are failing, could you please check them? |
|
The OTP-matrix failures were the connection tests asserting |
|
@mvanhorn could you resolve the merge conflicts? |
A connection that reports ready but cannot hand out a channel stays under
GRPC.Client.Supervisor forever: connect/2 returns {:error, :no_connection}
to the caller and nothing terminates the child. Terminate it on the
finalize-failure path, mirroring the existing terminate_child on the
ready_status error path.
Also give GRPC.Client.Supervisor hibernate_after and a fullsweep_after
spawn_opt so long-lived connection processes release binary-heavy heaps
instead of accumulating them.
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Adds a resolver that establishes and then drops its LB table so connect/2 takes the finalize-failure path, and asserts the child is gone afterwards. Supervisor assertions capture the pre-existing child count in setup rather than asserting zero on the shared GRPC.Client.Supervisor, which fails under the full suite where other tests leave children behind. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
5a6f752 to
e4fdaae
Compare
|
Rebased onto master — conflicts resolved, Worth flagging what the rebase surfaced: #557 already added I also dropped my Remaining diff is the finalize-path cleanup plus a |
| [ | ||
| name: GRPC.Client.Supervisor, | ||
| hibernate_after: 15_000, | ||
| spawn_opt: [fullsweep_after: 20] | ||
| ]} | ||
| ] |
There was a problem hiding this comment.
I wonder if these should be configurable via Application env
| # A connection that reported ready but cannot hand out a | ||
| # channel is unusable; leaving it under the supervisor is the | ||
| # leak this fixes. | ||
| _ = DynamicSupervisor.terminate_child(GRPC.Client.Supervisor, pid) |
There was a problem hiding this comment.
Should we hard-match on the result?
| end | ||
| end | ||
|
|
||
| defp assert_eventually(fun, 0), do: assert(fun.()) |
There was a problem hiding this comment.
Let's not add asssert_eventually in favor of more robust strategies. I believe waiting for the DOWN message should be enough with other assertions happening after that.
3 participants
Summary
Short-lived client connections no longer leak
GRPC.Client.Supervisorchildren. Orchestrator processes that fail to finalize a connection are terminated instead of being left under the DynamicSupervisor, and disconnected connections are fully cleaned up, so the supervisor's binary memory stays flat under connect/disconnect churn.Why this matters
Reported in #531: a user running hundreds of short-lived connections (
GRPC.Stub.connect/2thenGRPC.Stub.disconnect/1) on 1.0.0-rc.1 seesGRPC.Client.Supervisortop:recon.proc_count(:memory, 10)and:recon.bin_leak(10)with persistent growth; downgrading to 0.10.2 (no DynamicSupervisor in the client path) eliminates it, and a collaborator confirmed the diagnosis and labeled it a bug. The regression is in the 0.11+/1.0 client connection architecture:GRPC.Client.Connection.connect/2leaves the orchestrator child running whenfinalize_connection/2returns{:error, :no_connection}, and the disconnect path leaves per-connection state behind.Changes
connect/2now terminates the orchestrator child when connection finalization fails, so error paths cannot accumulate supervised processes.GRPC.Client.Supervisor.Testing
Added regression tests that drive repeated connect/disconnect cycles and connect-failure cycles, asserting
DynamicSupervisor.count_children/1onGRPC.Client.Supervisorreturns to baseline after each cycle. Ran the client connection test files with mix test.Fixes #531