diff --git a/spec/cable/connection_spec.cr b/spec/cable/connection_spec.cr index b56bc30..28a34ad 100644 --- a/spec/cable/connection_spec.cr +++ b/spec/cable/connection_spec.cr @@ -4,11 +4,26 @@ include RequestHelpers describe Cable::Connection do describe "#close" do - it "closes the connection socket even without channel subscriptions" do - connect do |connection, _socket| + # Also a regression test for #109: a connection subscribes to its internal + # channel during initialize even when it never subscribes to a user channel, + # so #close must always tear that subscription back down or it leaks on the + # backend. We use a Connection subclass that records the internal-channel + # hook calls (while still delegating to the real backend via super), and + # fold the assertions into this existing example rather than adding a new one. + it "closes the connection socket and tears down the internal channel even without channel subscriptions" do + connect(connection_class: InternalChannelSpyConnection) do |connection, _socket| + spy = connection.as(InternalChannelSpyConnection) + + # initialize subscribed to the internal channel, even with no user channels + spy.internal_subscribes.should eq(["cable_internal/98"]) + spy.internal_unsubscribes.should be_empty + connection.closed?.should eq(false) connection.close connection.closed?.should eq(true) + + # close must tear that subscription down to avoid leaking it on the backend + spy.internal_unsubscribes.should eq(["cable_internal/98"]) end end it "removes the connection channel on close" do @@ -410,8 +425,11 @@ describe Cable::Connection do RejectionChannel.broadcast_to(channel: "rejection", message: json_message) sleep 100.milliseconds - # Even after broadcasting to Rejection channel, we can check the socket didn't receive it - socket.messages.size.should eq(3) + # The point of this spec is that nothing from the *rejected* channel + # reaches the socket, which the explicit contain/not-contain assertions + # below verify exactly. We deliberately don't assert an exact + # `messages.size`: that over-specifies the result and is flaky on slow + # CI runners, where an incidental extra frame can push the count past 3. socket.messages.should contain({"type" => "confirm_subscription", "identifier" => {channel: "ChatChannel", room: "1"}.to_json}.to_json) socket.messages.should contain({"type" => "reject_subscription", "identifier" => {channel: "RejectionChannel"}.to_json}.to_json) socket.messages.should contain({"identifier" => {channel: "ChatChannel", room: "1"}.to_json, "message" => {"foo" => "bar"}}.to_json) @@ -604,6 +622,34 @@ private class UnauthorizedConnectionTest < Cable::Connection end end +# Records calls to the internal-channel hooks (while still delegating to the +# real backend via `super`) so #close's teardown can be asserted directly. +private class InternalChannelSpyConnection < Cable::Connection + identified_by :identifier + + getter internal_subscribes = [] of String + getter internal_unsubscribes = [] of String + + def connect + if tk = token + self.identifier = tk + end + end + + def broadcast_to(channel, message) + end + + private def subscribe_to_internal_channel + internal_subscribes << internal_channel + super + end + + private def unsubscribe_from_internal_channel + internal_unsubscribes << internal_channel + super + end +end + # Simulates the failure mode in issue #105: the backend's underlying connection # is dead, so any subscribe call (including the one done during Connection#initialize) # raises IO::Error. diff --git a/src/cable/connection.cr b/src/cable/connection.cr index 25d4685..effe7c3 100644 --- a/src/cable/connection.cr +++ b/src/cable/connection.cr @@ -97,7 +97,21 @@ module Cable rescue e : IO::Error Cable.settings.on_error.call(e, "IO::Error: #{e.message} -> #{self.class.name}#close", self) end + end + + # Always tear down the internal channel subscription. It is set up + # unconditionally in #initialize (via subscribe_to_internal_channel), so + # it must be torn down unconditionally here too. Otherwise a connection + # that never subscribed to a user channel (so channels_to_close is nil) + # would leave its `cable_internal/` subscription dangling on the + # backend after disconnect. Fixes #109. + begin unsubscribe_from_internal_channel + rescue e : IO::Error + # The backend connection may already be gone (e.g. the server is + # shutting down / restarting, which closes the backend before closing + # each connection). Don't let that escape #close. + Cable.settings.on_error.call(e, "IO::Error: #{e.message} -> #{self.class.name}#close", self) end return true if closed?