Treat HTTP 304 as idempotent success for container start/stop (#13)

* Treat non-redirect 304 responses as API status errors

* Treat Docker 304 as success for container start/stop

Author: corsicanec82

lib/docker_engine_ruby/internal/transport/base_client.rb

@@ -11,6 +11,7 @@ class BaseClient
 
         # from whatwg fetch spec
         MAX_REDIRECTS = 20
+        REDIRECT_STATUSES = [301, 302, 303, 307, 308].freeze
 
         # rubocop:disable Style/MutableConstant
         PLATFORM_HEADERS =
@@ -67,6 +68,15 @@ def should_retry?(status, headers:)
             end
           end
 
+          # @api private
+          #
+          # @param status [Integer]
+          #
+          # @return [Boolean]
+          def redirect_status?(status)
+            REDIRECT_STATUSES.include?(status)
+          end
+
           # @api private
           #
           # @param request [Hash{Symbol=>Object}] .
@@ -383,7 +393,8 @@ def send_request(request, redirect_count:, retry_count:, send_retry_header:)
           case status
           in ..299
             [status, response, stream]
-          in 300..399 if redirect_count >= self.class::MAX_REDIRECTS
+          in Integer => status if self.class.redirect_status?(status) &&
+            redirect_count >= self.class::MAX_REDIRECTS
             self.class.reap_connection!(status, stream: stream)
 
             message = "Failed to complete the request within #{self.class::MAX_REDIRECTS} redirects."
@@ -392,7 +403,7 @@ def send_request(request, redirect_count:, retry_count:, send_retry_header:)
               response: response,
               message: message
             )
-          in 300..399
+          in Integer => status if self.class.redirect_status?(status)
             self.class.reap_connection!(status, stream: stream)
 
             request = self.class.follow_redirect(request, status: status, response_headers: headers)
@@ -404,7 +415,7 @@ def send_request(request, redirect_count:, retry_count:, send_retry_header:)
             )
           in DockerEngineRuby::Errors::APIConnectionError if retry_count >= max_retries
             raise status
-          in (400..) if retry_count >= max_retries || !self.class.should_retry?(status, headers: headers)
+          in (300..) if retry_count >= max_retries || !self.class.should_retry?(status, headers: headers)
             decoded = Kernel.then do
               DockerEngineRuby::Internal::Util.decode_content(headers, stream: stream, suppress_error: true)
             ensure

lib/docker_engine_ruby/resources/containers.rb

@@ -483,6 +483,9 @@ def start(id, params = {})
           model: NilClass,
           options: options
         )
+      rescue DockerEngineRuby::Errors::APIStatusError => e
+        raise e unless e.status == 304
+        nil
       end
 
       # Get container stats based on resource usage
@@ -529,6 +532,9 @@ def stop(id, params = {})
           model: NilClass,
           options: options
         )
+      rescue DockerEngineRuby::Errors::APIStatusError => e
+        raise e unless e.status == 304
+        nil
       end
 
       # List processes running inside a container

test/docker_engine_ruby/client_test.rb

@@ -296,6 +296,36 @@ def test_images_pull_with_json_lines_response_returns_nil
     assert_nil(response)
   end
 
+  def test_containers_start_304_is_treated_as_success
+    stub_request(:post, "http://localhost/containers/id/start").to_return(status: 304, body: "")
+
+    docker = DockerEngineRuby::Client.new(base_url: "http://localhost")
+
+    response = docker.containers.start("id")
+
+    assert_nil(response)
+  end
+
+  def test_containers_stop_304_is_treated_as_success
+    stub_request(:post, "http://localhost/containers/id/stop").to_return(status: 304, body: "")
+
+    docker = DockerEngineRuby::Client.new(base_url: "http://localhost")
+
+    response = docker.containers.stop("id")
+
+    assert_nil(response)
+  end
+
+  def test_non_redirect_304_is_treated_as_status_error_for_other_operations
+    stub_request(:get, "http://localhost/containers/json").to_return(status: 304, body: "")
+
+    docker = DockerEngineRuby::Client.new(base_url: "http://localhost")
+
+    error = assert_raises(DockerEngineRuby::Errors::APIStatusError) { docker.containers.list }
+
+    assert_equal(304, error.status)
+  end
+
   def test_default_headers
     stub_request(:get, "http://localhost/containers/json").to_return_json(status: 200, body: {})