Skip to content

Commit bb48fbe

Browse files
russclaude
andcommitted
Always unsubscribe from the internal channel on Connection#close
A connection subscribes to its internal channel unconditionally during #initialize (via subscribe_to_internal_channel), but #close only tore that subscription down inside the `if channels_to_close` branch. A connection that opened but never subscribed to a user channel therefore left its `cable_internal/<id>` subscription dangling on the backend after disconnecting — a small but real leak. Move unsubscribe_from_internal_channel out of that branch so it always runs on close, mirroring the unconditional subscribe in #initialize. Because #close now always reaches the backend, guard the call against IO::Error: Server#shutdown closes the backend connections *before* closing each Connection, so during a restart/shutdown the unsubscribe can hit an already-closed backend. Without the guard that IO::Error escapes #close and breaks the shutdown loop (it surfaced as the handler "restarts the server if too many errors" spec failing). This mirrors the existing defensive handling in #initialize and Server#shutdown. Adds regression coverage to the existing "#close ... without channel subscriptions" example via a Connection subclass that records the internal-channel hook calls while still delegating to the real backend. Fixes #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BMW1EsdCMExBhm3tg7TQhs
1 parent bfdb55d commit bb48fbe

2 files changed

Lines changed: 59 additions & 2 deletions

File tree

spec/cable/connection_spec.cr

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,26 @@ include RequestHelpers
44

55
describe Cable::Connection do
66
describe "#close" do
7-
it "closes the connection socket even without channel subscriptions" do
8-
connect do |connection, _socket|
7+
# Also a regression test for #109: a connection subscribes to its internal
8+
# channel during initialize even when it never subscribes to a user channel,
9+
# so #close must always tear that subscription back down or it leaks on the
10+
# backend. We use a Connection subclass that records the internal-channel
11+
# hook calls (while still delegating to the real backend via super), and
12+
# fold the assertions into this existing example rather than adding a new one.
13+
it "closes the connection socket and tears down the internal channel even without channel subscriptions" do
14+
connect(connection_class: InternalChannelSpyConnection) do |connection, _socket|
15+
spy = connection.as(InternalChannelSpyConnection)
16+
17+
# initialize subscribed to the internal channel, even with no user channels
18+
spy.internal_subscribes.should eq(["cable_internal/98"])
19+
spy.internal_unsubscribes.should be_empty
20+
921
connection.closed?.should eq(false)
1022
connection.close
1123
connection.closed?.should eq(true)
24+
25+
# close must tear that subscription down to avoid leaking it on the backend
26+
spy.internal_unsubscribes.should eq(["cable_internal/98"])
1227
end
1328
end
1429
it "removes the connection channel on close" do
@@ -604,6 +619,34 @@ private class UnauthorizedConnectionTest < Cable::Connection
604619
end
605620
end
606621

622+
# Records calls to the internal-channel hooks (while still delegating to the
623+
# real backend via `super`) so #close's teardown can be asserted directly.
624+
private class InternalChannelSpyConnection < Cable::Connection
625+
identified_by :identifier
626+
627+
getter internal_subscribes = [] of String
628+
getter internal_unsubscribes = [] of String
629+
630+
def connect
631+
if tk = token
632+
self.identifier = tk
633+
end
634+
end
635+
636+
def broadcast_to(channel, message)
637+
end
638+
639+
private def subscribe_to_internal_channel
640+
internal_subscribes << internal_channel
641+
super
642+
end
643+
644+
private def unsubscribe_from_internal_channel
645+
internal_unsubscribes << internal_channel
646+
super
647+
end
648+
end
649+
607650
# Simulates the failure mode in issue #105: the backend's underlying connection
608651
# is dead, so any subscribe call (including the one done during Connection#initialize)
609652
# raises IO::Error.

src/cable/connection.cr

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,21 @@ module Cable
9797
rescue e : IO::Error
9898
Cable.settings.on_error.call(e, "IO::Error: #{e.message} -> #{self.class.name}#close", self)
9999
end
100+
end
101+
102+
# Always tear down the internal channel subscription. It is set up
103+
# unconditionally in #initialize (via subscribe_to_internal_channel), so
104+
# it must be torn down unconditionally here too. Otherwise a connection
105+
# that never subscribed to a user channel (so channels_to_close is nil)
106+
# would leave its `cable_internal/<id>` subscription dangling on the
107+
# backend after disconnect. Fixes #109.
108+
begin
100109
unsubscribe_from_internal_channel
110+
rescue e : IO::Error
111+
# The backend connection may already be gone (e.g. the server is
112+
# shutting down / restarting, which closes the backend before closing
113+
# each connection). Don't let that escape #close.
114+
Cable.settings.on_error.call(e, "IO::Error: #{e.message} -> #{self.class.name}#close", self)
101115
end
102116

103117
return true if closed?

0 commit comments

Comments
 (0)