Skip to content

Fix event-task leak with a counter + drain promise (WaitGroup) — alt to #15#16

Closed
henrikbjorn wants to merge 1 commit into
masterfrom
fix-event-leak-waitgroup
Closed

Fix event-task leak with a counter + drain promise (WaitGroup) — alt to #15#16
henrikbjorn wants to merge 1 commit into
masterfrom
fix-event-leak-waitgroup

Conversation

@henrikbjorn

Copy link
Copy Markdown
Member

Alternative to #15 (opened so both approaches can be compared side by side). Same bug, same regression test, different drain mechanism.

The bug (same as #15)

Listener::Base dispatched every ESL event through one per-connection Async::Barrier. Barrier#async retains each task until #wait, and @event_barrier.wait is only called in connection_closed (socket drop). So on a long-lived inbound connection every event permanently leaks a task node — ~GBs over days on a busy switch (the answering_machine ~8GB/2d).

This approach: counter + drain Promise (WaitGroup)

Keep only a count of in-flight event tasks plus a one-shot drain Async::Promise:

@event_tasks = 0
@drained = Async::Promise.new
@closing = false

# per event:
@event_tasks += 1
Async do
  on_event(response); invoke_event_hooks(response)
ensure
  @event_tasks -= 1
  @drained.resolve(true) if @closing && @event_tasks.zero?
end

# connection_closed (after rejecting pending promises):
@closing = true
@drained.resolve(true) if @event_tasks.zero?
@drained.wait

Retention is one integer for the whole life of the connection — nothing per-event is stored.

vs the Set approach (#15)

Both fix the leak and pass the same regression test. This one:

  • stores a count, not a collection — even leaner;
  • has no snapshot/iterate on drain (no .dup);
  • can't truncate the drain on a hook error (the Set's each(&:wait) re-raises the first failure; Barrier#wait had the same gap).

Promise (not Condition) is deliberate: a resolve that races ahead of the wait is still delivered, so shutdown ordering can't lose the signal.

This mirrors the design already on the spike/em-style-simplification branch (33f6fbd); this PR just lands it minimally on master. Full suite green (121 runs, 0 failures).

Alternative to the Set-based fix: instead of tracking in-flight event-hook
tasks in a collection, keep only a count (@event_tasks) plus a one-shot drain
Promise. Each event increments before spawning and decrements in its ensure;
connection_closed sets @closing, and the last hook to finish (or connection_closed
itself, if already idle) resolves the promise, which the drain awaits.

Retention is a single integer for the whole life of the connection — nothing
per-event is stored — vs the old Async::Barrier, which retained one task node
per event until #wait (called only on disconnect), leaking ~GBs over days on a
long-lived inbound connection subscribed to many events.

vs the Set approach: no collection to snapshot/iterate on drain, and no
first-error truncation of the drain loop. Same regression test (in-flight count
returns to zero). Full suite green.
@henrikbjorn

Copy link
Copy Markdown
Member Author

Closing in favour of #15 (self-reaping Set). Both fix the leak identically and pass the same test; #15 keeps less coordinated state (one collection that self-reaps, vs this PR's counter + drain-promise + closing-flag). Keeping the simpler one. #15 now uses @event_tasks.each { it.wait rescue nil } for the drain (no snapshot needed — only deletes happen during shutdown, which MRI tolerates).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant