Merge pull request #300 from github/reuse-graphql-http

Create a per-org shared connection for graphql requests

Author: hosom

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    entitlements-github-plugin (1.2.3)
+    entitlements-github-plugin (1.2.4)
       contracts (~> 0.17.0)
       faraday (~> 2.0)
       faraday-retry (~> 2.0)

lib/entitlements/service/github.rb

@@ -369,17 +369,15 @@ def graphql_result_retryable?(result)
       # Returns { code: <Integer>, data: <response data structure> }
       Contract String => { code: Integer, data: C::Or[nil, Hash] }
       def graphql_http_post_real(query)
-        uri = URI.parse(File.join(octokit.api_endpoint, "graphql"))
-        http = Net::HTTP.new(uri.host, uri.port)
-        http.use_ssl = uri.scheme == "https"
+        uri = graphql_uri
 
         request = Net::HTTP::Post.new(uri)
         request.add_field("Authorization", "bearer #{token}")
         request.add_field("Content-Type", "application/json")
         request.body = JSON.generate("query" => query)
 
         begin
-          response = http.request(request)
+          response = graphql_http.request(request)
 
           if response.code != "200"
             # The retry wrapper retries on 5xx, so log those at WARN to avoid misleading
@@ -411,11 +409,41 @@ def graphql_http_post_real(query)
           # Catch-all for any unexpected exception (network blip OR local code bug).
           # We retry below via the synthesized 500, but log at ERROR because this
           # branch can mask programming errors that operators must see.
+          reset_graphql_http
           Entitlements.logger.error "Caught #{e.class} POSTing to #{uri}: #{e.message}"
           { code: 500, data: nil }
         end
       end
 
+      # Return the URI used for GraphQL requests.
+      Contract C::None => URI::Generic
+      def graphql_uri
+        @graphql_uri ||= URI.parse(File.join(octokit.api_endpoint, "graphql"))
+      end
+
+      # Return a started HTTP connection that can be reused across GraphQL requests.
+      Contract C::None => Net::HTTP
+      def graphql_http
+        @graphql_http ||= begin
+          uri = graphql_uri
+          http = Net::HTTP.new(uri.host, uri.port)
+          http.use_ssl = uri.scheme == "https"
+          http.start
+          http
+        end
+      end
+
+      # Discard a failed GraphQL connection so the retry wrapper creates a new one.
+      Contract C::None => nil
+      def reset_graphql_http
+        http, @graphql_http = @graphql_http, nil
+        http.finish if http&.started?
+        nil
+      rescue IOError, SystemCallError => e
+        Entitlements.logger.warn "Failed to close GraphQL connection: #{e.message}"
+        nil
+      end
+
       # Create a unique signature for this GitHub instance to identify it in a global cache.
       #
       # Takes no arguments.

lib/version.rb

@@ -2,6 +2,6 @@
 
 module Entitlements
   module Version
-    VERSION = "1.2.3"
+    VERSION = "1.2.4"
   end
 end

spec/unit/entitlements/service/github_spec.rb

@@ -312,6 +312,33 @@
     let(:answer) { { "foo" => "bar" } }
     let(:query) { "my query here" }
 
+    it "reconnects and retries after the connection times out" do
+      response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer))
+      timed_out_http = Net::HTTP.new("github.fake", 443)
+      replacement_http = Net::HTTP.new("github.fake", 443)
+
+      expect(Net::HTTP).to receive(:new).
+        with("github.fake", 443).
+        twice.
+        and_return(timed_out_http, replacement_http)
+      expect(timed_out_http).to receive(:use_ssl=).with(true)
+      expect(timed_out_http).to receive(:start)
+      expect(timed_out_http).to receive(:request).
+        and_raise(Net::ReadTimeout, "execution expired")
+      expect(timed_out_http).to receive(:started?).and_return(true)
+      expect(timed_out_http).to receive(:finish)
+      expect(replacement_http).to receive(:use_ssl=).with(true)
+      expect(replacement_http).to receive(:start)
+      expect(replacement_http).to receive(:request).and_return(response)
+      expect_any_instance_of(Object).to receive(:sleep).with(1).once
+      expect(logger).to receive(:error).
+        with("Caught Net::ReadTimeout POSTing to https://github.fake/api/v3/graphql: Net::ReadTimeout with \"execution expired\"")
+      expect(logger).to receive(:warn).
+        with("GraphQL failed on try 1 of 3. Will retry.")
+
+      expect(subject.send(:graphql_http_post, query)).to eq(code: 200, data: answer)
+    end
+
     it "returns immediately for success" do
       expect(subject).to receive(:graphql_http_post_real).with(query).and_return(code: 200, data: answer)
       expect_any_instance_of(Object).not_to receive(:sleep)
@@ -361,6 +388,52 @@
   end
 
   describe "#graphql_http_post_real" do
+    it "reuses a started HTTP connection across requests" do
+      answer = { "foo" => "bar" }
+      response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer))
+      http = Net::HTTP.new("github.fake", 443)
+
+      expect(Net::HTTP).to receive(:new).with("github.fake", 443).once.and_return(http)
+      expect(http).to receive(:use_ssl=).with(true).once
+      expect(http).to receive(:start).once
+      expect(http).to receive(:request).twice.and_return(response)
+
+      2.times do
+        expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 200, data: answer)
+      end
+    end
+
+    it "discards a failed connection so the next request reconnects" do
+      answer = { "foo" => "bar" }
+      response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer))
+      failed_http = Net::HTTP.new("github.fake", 443)
+      replacement_http = Net::HTTP.new("github.fake", 443)
+
+      expect(Net::HTTP).to receive(:new).with("github.fake", 443).twice.and_return(failed_http, replacement_http)
+      expect(failed_http).to receive(:use_ssl=).with(true)
+      expect(failed_http).to receive(:start)
+      expect(failed_http).to receive(:request).and_raise(EOFError, "closed connection")
+      expect(failed_http).to receive(:started?).and_return(true)
+      expect(failed_http).to receive(:finish)
+      expect(replacement_http).to receive(:use_ssl=).with(true)
+      expect(replacement_http).to receive(:start)
+      expect(replacement_http).to receive(:request).and_return(response)
+      expect(logger).to receive(:error).
+        with("Caught EOFError POSTing to https://github.fake/api/v3/graphql: closed connection")
+
+      expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 500, data: nil)
+      expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 200, data: answer)
+    end
+
+    it "logs a warning if a failed connection cannot be closed" do
+      http = instance_double(Net::HTTP, started?: true)
+      subject.instance_variable_set(:@graphql_http, http)
+
+      expect(http).to receive(:finish).and_raise(IOError, "already closed")
+      expect(logger).to receive(:warn).with("Failed to close GraphQL connection: already closed")
+      expect(subject.send(:reset_graphql_http)).to be_nil
+    end
+
     it "returns code=200 and parsed JSON for a successful response" do
       answer = { "foo" => ["bar", "baz" => "fizz"] }
       stub_request(:post, "https://github.fake/api/v3/graphql").to_return(status: 200, body: JSON.generate(answer))