Skip to content

Commit 54c1774

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. The method is already guarded by `internal_identifier.presence` and is a no-op on the backend when not subscribed, so calling it unconditionally is safe. Adds a regression spec that spies on the internal-channel hooks to assert #close tears the subscription down even with no user-channel subscriptions. Fixes #109 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BMW1EsdCMExBhm3tg7TQhs
1 parent bfdb55d commit 54c1774

2 files changed

Lines changed: 56 additions & 1 deletion

File tree

spec/cable/connection_spec.cr

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,28 @@ describe Cable::Connection do
198198
end
199199
end
200200

201+
# Regression test for #109: a connection that opens but never subscribes to a
202+
# user channel still subscribes to its internal channel during initialize. On
203+
# close it must unsubscribe from that internal channel, otherwise the
204+
# subscription leaks on the backend. We spy on the internal-channel hooks so
205+
# the test exercises Connection#close's control flow directly, without
206+
# swapping the global server backend (which destabilizes other specs).
207+
describe "when a connection closes without ever subscribing to a channel" do
208+
it "still unsubscribes from the internal channel (no leak)" do
209+
socket = DummySocket.new(IO::Memory.new)
210+
connection = InternalChannelSpyConnection.new(builds_request(token: "98"), socket)
211+
212+
# initialize subscribes to the internal channel, even with no user channels
213+
connection.internal_subscribes.should eq(["cable_internal/98"])
214+
connection.internal_unsubscribes.should be_empty
215+
216+
connection.close
217+
218+
# close must tear that subscription down to avoid leaking it on the backend
219+
connection.internal_unsubscribes.should eq(["cable_internal/98"])
220+
end
221+
end
222+
201223
describe "#message" do
202224
it "ignore a message for a non valid channel" do
203225
connect do |connection, socket|
@@ -604,6 +626,32 @@ private class UnauthorizedConnectionTest < Cable::Connection
604626
end
605627
end
606628

629+
# Records calls to the internal-channel hooks instead of hitting the backend,
630+
# so #close's teardown can be asserted without touching the global server.
631+
private class InternalChannelSpyConnection < Cable::Connection
632+
identified_by :identifier
633+
634+
getter internal_subscribes = [] of String
635+
getter internal_unsubscribes = [] of String
636+
637+
def connect
638+
if tk = token
639+
self.identifier = tk
640+
end
641+
end
642+
643+
def broadcast_to(channel, message)
644+
end
645+
646+
private def subscribe_to_internal_channel
647+
internal_subscribes << internal_channel
648+
end
649+
650+
private def unsubscribe_from_internal_channel
651+
internal_unsubscribes << internal_channel
652+
end
653+
end
654+
607655
# Simulates the failure mode in issue #105: the backend's underlying connection
608656
# is dead, so any subscribe call (including the one done during Connection#initialize)
609657
# raises IO::Error.

src/cable/connection.cr

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,16 @@ 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-
unsubscribe_from_internal_channel
101100
end
102101

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+
unsubscribe_from_internal_channel
109+
103110
return true if closed?
104111

105112
Cable::Logger.info { "Terminating connection #{connection_identifier}" }

0 commit comments

Comments
 (0)