diff --git a/Gemfile.lock b/Gemfile.lock index b527a36..b20fa84 100644 --- a/Gemfile.lock +++ b/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) diff --git a/lib/entitlements/service/github.rb b/lib/entitlements/service/github.rb index 0a977a9..ff75edd 100644 --- a/lib/entitlements/service/github.rb +++ b/lib/entitlements/service/github.rb @@ -369,9 +369,7 @@ def graphql_result_retryable?(result) # Returns { code: , data: } 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}") @@ -379,7 +377,7 @@ def graphql_http_post_real(query) 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. diff --git a/lib/version.rb b/lib/version.rb index cd6c58b..6908551 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -2,6 +2,6 @@ module Entitlements module Version - VERSION = "1.2.3" + VERSION = "1.2.4" end end diff --git a/spec/unit/entitlements/service/github_spec.rb b/spec/unit/entitlements/service/github_spec.rb index eea708e..7a61aca 100644 --- a/spec/unit/entitlements/service/github_spec.rb +++ b/spec/unit/entitlements/service/github_spec.rb @@ -361,6 +361,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))