From 035eda1f95aefc124947c2c35de873d3ee44abfc Mon Sep 17 00:00:00 2001
From: HoneyryderChuck <cardoso_tiago@hotmail.com>
Date: Fri, 27 Nov 2020 12:42:41 +0000
Subject: [PATCH] fixed another loop caused by certain connection goaway frames
 from server not being processed

While introducing yet another test to catch frame processing errors, in
this case with the SETTINGS_TIMEOUT error, another loop was found. It
was caused by two reasons:

* connection was signaling it was "closing" on such an error, which is
  not really true (server already closed the stream, so no need to
  close it again); it should be marked as closed instead.
* write buffer was still full (with the handshake in this case), so the
  connection was still trying to write;
---
 lib/httpx/connection/http2.rb              | 10 +++++-----
 test/https_test.rb                         | 10 ++++++++++
 test/support/session_with_frame_delay.rb   | 14 ++++++++++++++
 test/support/session_with_single_stream.rb |  3 +--
 4 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 test/support/session_with_frame_delay.rb

diff --git a/lib/httpx/connection/http2.rb b/lib/httpx/connection/http2.rb
index fb533245..a01dd6ad 100644
--- a/lib/httpx/connection/http2.rb
+++ b/lib/httpx/connection/http2.rb
@@ -269,16 +269,16 @@ module HTTPX
     end
 
     def on_close(_last_frame, error, _payload)
+      is_connection_closed = @connection.state == :closed
       if error && error != :no_error
+        @buffer.clear if is_connection_closed
         ex = Error.new(0, error)
         ex.set_backtrace(caller)
-        @streams.each_key do |request|
-          emit(:error, request, ex)
-        end
+        handle_error(ex)
       end
-      return unless @connection.state == :closed && @streams.size.zero?
+      return unless is_connection_closed && @streams.size.zero?
 
-      emit(:close)
+      emit(:close, is_connection_closed)
     end
 
     def on_frame_sent(frame)
diff --git a/test/https_test.rb b/test/https_test.rb
index c9b7af79..b37148b2 100644
--- a/test/https_test.rb
+++ b/test/https_test.rb
@@ -98,6 +98,16 @@ class HTTPSTest < Minitest::Test
         assert response.version == "1.1", "request should have been retried with HTTP/1.1"
       end
     end
+
+    def test_http2_settings_timeout
+      uri = build_uri("/get")
+      HTTPX.plugin(SessionWithPool).plugin(SessionWithFrameDelay).wrap do |http|
+        response = http.get(uri)
+        assert response.is_a?(HTTPX::ErrorResponse), "expected to fail for settings timeout"
+        assert response.status =~ /settings_timeout/,
+               "connection should have terminated due to HTTP/2 settings timeout"
+      end
+    end
   end
 
   private
diff --git a/test/support/session_with_frame_delay.rb b/test/support/session_with_frame_delay.rb
new file mode 100644
index 00000000..41341280
--- /dev/null
+++ b/test/support/session_with_frame_delay.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+#
+# This module is used only to test frame errors for HTTP/2. It targets the settings timeout of
+# nghttp2.org, which is known as being 10 seconnds.
+#
+module SessionWithFrameDelay
+  module ConnectionMethods
+    def send_pending
+      sleep(11)
+      super
+    end
+  end
+end
diff --git a/test/support/session_with_single_stream.rb b/test/support/session_with_single_stream.rb
index 98c9a91e..85c12706 100644
--- a/test/support/session_with_single_stream.rb
+++ b/test/support/session_with_single_stream.rb
@@ -12,8 +12,7 @@ module SessionWithSingleStream
         @connection.active_stream_count.positive?
       end
       parser.instance_variable_set(:@max_requests, 10)
-      connection = parser.instance_variable_get(:@connection)
-      connection.max_streams = 1
+      parser.max_streams = 1
       parser
     end
   end