Breaking public API change: Make Bunny::Channel#close raise an except…

…ion on a closed channel

The current behaviour is to time out on a network request, which makes no sense
at all, as #528 explains. This is also what other clients do. Making the method
a no-op was also an option but doing so might make genuine channel-level exceptions
harder to discover for the user.

While at it, report the last seen channel.close exception. This is both
informative and consistent with what RabbitMQ Java client does: it
provides a "shutdown reason" (e.g. a protocol exception if any) together
with the "already closed" exception.

Since this is a minor breaking public API change, it won't be backported
to 2.7.x.

Fixes #528.

lib/bunny/channel.rb

@@ -233,6 +233,9 @@ def open
     # {Bunny::Queue}, {Bunny::Exchange} and {Bunny::Consumer} instances.
     # @api public
     def close
+      # see bunny#528
+      raise_if_no_longer_open!
+
       @connection.close_channel(self)
       @status = :closed
       @work_pool.shutdown
@@ -1940,7 +1943,13 @@ def raise_if_continuation_resulted_in_a_channel_error!
 
     # @private
     def raise_if_no_longer_open!
-      raise ChannelAlreadyClosed.new("cannot use a channel that was already closed! Channel id: #{@id}", self) if closed?
+      if closed?
+        if @last_channel_error
+          raise ChannelAlreadyClosed.new("cannot use a closed channel! Channel id: #{@id}, closed due to a server-reported channel error: #{@last_channel_error.message}", self)
+        else
+          raise ChannelAlreadyClosed.new("cannot use a closed channel! Channel id: #{@id}", self)
+        end
+      end
     end
 
     # @private

spec/higher_level_api/integration/channel_close_spec.rb

@@ -1,6 +1,6 @@
 require "spec_helper"
 
-describe Bunny::Channel, "when closed" do
+describe Bunny::Channel do
   let(:connection) do
     c = Bunny.new(username: "bunny_gem", password: "bunny_password", vhost: "bunny_testbed")
     c.start
@@ -11,15 +11,52 @@
     connection.close
   end
 
-  it "releases the id" do
-    ch = connection.create_channel
-    n = ch.number
+  context "when closed" do
+    it "releases the id" do
+      ch = connection.create_channel
+      n = ch.number
 
-    expect(ch).to be_open
-    ch.close
-    expect(ch).to be_closed
+      expect(ch).to be_open
+      ch.close
+      expect(ch).to be_closed
 
-    # a new channel with the same id can be created
-    connection.create_channel(n)
+      # a new channel with the same id can be created
+      connection.create_channel(n)
+    end
+  end
+
+  context "when double closed" do
+    # bunny#528
+    it "raises a meaningful exception" do
+      ch = connection.create_channel
+
+      expect(ch).to be_open
+      ch.close
+      expect(ch).to be_closed
+
+      expect { ch.close }.to raise_error(Bunny::ChannelAlreadyClosed)
+    end
+  end
+
+  context "when double closed after a channel-level protocol exception" do
+    # bunny#528
+    it "raises a meaningful exception" do
+      ch = connection.create_channel
+
+      s  = "bunny-temp-q-#{rand}"
+
+      expect(ch).to be_open
+      ch.queue_declare(s, durable: false, exclusive: true)
+
+      expect do
+        ch.queue_declare(s, durable: true, exclusive: false)
+      end.to raise_error(Bunny::PreconditionFailed)
+
+      # channel.close is sent and handled concurrently with the test
+      sleep 1
+      expect(ch).to be_closed
+
+      expect { ch.close }.to raise_error(Bunny::ChannelAlreadyClosed)
+    end
   end
 end