fix: stop leaking client supervisor children on failed and closed connections#554
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 |
| # 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.
- Make hibernate_after / fullsweep_after configurable via Application env, defaulting to the previous hardcoded values. - Hard-match the DynamicSupervisor.terminate_child result on the finalize_connection error path. - Drop assert_eventually in favor of monitoring the connection process and waiting on the :DOWN message before asserting registry and child counts. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
The OTP matrix was failing at connection_test.exs:155 with the registry still holding the entry after the connection process had exited. Registry removes an entry from its own monitor of the registered process. That monitor and the one the test installs both fire on the same exit with no ordering guarantee, so receiving :DOWN does not imply the registry has processed its copy. The read was simply racing the cleanup. A system call to the partition that owns the monitor drains that message before the table is read. A local harness that repeats the register / kill / read cycle 3000 times reports 303 stale reads unsynchronized, 0 with this barrier, so this is deterministic rather than a narrower window. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Good call on dropping Waiting on the d4ad461 keeps the The assertions after the |
| Application.put_env(:grpc, NoPickResolver, self()) | ||
| on_exit(fn -> Application.delete_env(:grpc, NoPickResolver) end) |
There was a problem hiding this comment.
Not for this PR, but I think this test suite needs refactoring so we can speed it up
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