]> cat aescling's git repositories - mastodon.git/commitdiff
Close http connection in perform method of Request class (#6889)
authorAkihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
Sat, 24 Mar 2018 11:49:54 +0000 (20:49 +0900)
committerEugen Rochko <eugen@zeonfederated.com>
Sat, 24 Mar 2018 11:49:54 +0000 (12:49 +0100)
HTTP connections must be explicitly closed in many cases, and letting
perform method close connections makes its callers less redundant and
prevent them from forgetting to close connections.

15 files changed:
app/helpers/jsonld_helper.rb
app/lib/provider_discovery.rb
app/lib/request.rb
app/models/concerns/remotable.rb
app/services/fetch_atom_service.rb
app/services/fetch_link_card_service.rb
app/services/resolve_account_service.rb
app/services/send_interaction_service.rb
app/services/subscribe_service.rb
app/services/unsubscribe_service.rb
app/workers/activitypub/delivery_worker.rb
app/workers/pubsubhubbub/confirmation_worker.rb
app/workers/pubsubhubbub/delivery_worker.rb
lib/tasks/mastodon.rake
spec/lib/request_spec.rb

index 9530ad9f30f7babeed890bc2eb0544234808949e..957a2cbc982a187425846235af9211ca0c1b8392 100644 (file)
@@ -60,9 +60,9 @@ module JsonLdHelper
   end
 
   def fetch_resource_without_id_validation(uri)
-    response = build_request(uri).perform
-    return if response.code != 200
-    body_to_json(response.to_s)
+    build_request(uri).perform do |response|
+      response.code == 200 ? body_to_json(response.to_s) : nil
+    end
   end
 
   def body_to_json(body)
index 5732e4fcb43943cd0ca89780272987ae5885d172..bbd3a2d43ef626db675c76858d14d84a3fe57a5f 100644 (file)
@@ -13,15 +13,14 @@ class ProviderDiscovery < OEmbed::ProviderDiscovery
     def discover_provider(url, **options)
       format = options[:format]
 
-      if options[:html]
-        html = Nokogiri::HTML(options[:html])
-      else
-        res = Request.new(:get, url).perform
-
-        raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html'
-
-        html = Nokogiri::HTML(res.to_s)
-      end
+      html = if options[:html]
+               Nokogiri::HTML(options[:html])
+             else
+               Request.new(:get, url).perform do |res|
+                 raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html'
+                 Nokogiri::HTML(res.to_s)
+               end
+             end
 
       if format.nil? || format == :json
         provider_endpoint ||= html.at_xpath('//link[@type="application/json+oembed"]')&.attribute('href')&.value
index 298fb9528faa54eb5f86315d5125903861188de2..8a127c65f2229016822346f5cb72a422450d0bba 100644 (file)
@@ -33,9 +33,17 @@ class Request
   end
 
   def perform
-    http_client.headers(headers).public_send(@verb, @url.to_s, @options)
-  rescue => e
-    raise e.class, "#{e.message} on #{@url}", e.backtrace[0]
+    begin
+      response = http_client.headers(headers).public_send(@verb, @url.to_s, @options)
+    rescue => e
+      raise e.class, "#{e.message} on #{@url}", e.backtrace[0]
+    end
+
+    begin
+      yield response
+    ensure
+      http_client.close
+    end
   end
 
   def headers
@@ -88,7 +96,7 @@ class Request
   end
 
   def http_client
-    HTTP.timeout(:per_operation, timeout).follow(max_hops: 2)
+    @http_client ||= HTTP.timeout(:per_operation, timeout).follow(max_hops: 2)
   end
 
   class Socket < TCPSocket
index 69685ec83af455a602b64ac32ba62aa4ed122d97..0f18c5d9637f850eda74137386052aa487226bfb 100644 (file)
@@ -21,23 +21,23 @@ module Remotable
         return if !%w(http https).include?(parsed_url.scheme) || parsed_url.host.empty? || self[attribute_name] == url
 
         begin
-          response = Request.new(:get, url).perform
-
-          return if response.code != 200
-
-          matches  = response.headers['content-disposition']&.match(/filename="([^"]*)"/)
-          filename = matches.nil? ? parsed_url.path.split('/').last : matches[1]
-          basename = SecureRandom.hex(8)
-          extname = if filename.nil?
-                      ''
-                    else
-                      File.extname(filename)
-                    end
-
-          send("#{attachment_name}=", StringIO.new(response.to_s))
-          send("#{attachment_name}_file_name=", basename + extname)
-
-          self[attribute_name] = url if has_attribute?(attribute_name)
+          Request.new(:get, url).perform do |response|
+            next if response.code != 200
+
+            matches  = response.headers['content-disposition']&.match(/filename="([^"]*)"/)
+            filename = matches.nil? ? parsed_url.path.split('/').last : matches[1]
+            basename = SecureRandom.hex(8)
+            extname = if filename.nil?
+                        ''
+                      else
+                        File.extname(filename)
+                      end
+
+            send("#{attachment_name}=", StringIO.new(response.to_s))
+            send("#{attachment_name}_file_name=", basename + extname)
+
+            self[attribute_name] = url if has_attribute?(attribute_name)
+          end
         rescue HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, Paperclip::Errors::NotIdentifiedByImageMagickError, Addressable::URI::InvalidURIError, Mastodon::HostValidationError => e
           Rails.logger.debug "Error fetching remote #{attachment_name}: #{e}"
           nil
index c0785984509466a19d37fb5ce4a5becba91e29fd..48ad5dcd37353bf4796d5384b9b4def5e8f8ebd9 100644 (file)
@@ -24,43 +24,44 @@ class FetchAtomService < BaseService
 
   def process(url, terminal = false)
     @url = url
-    perform_request
-    process_response(terminal)
+    perform_request { |response| process_response(response, terminal) }
   end
 
-  def perform_request
+  def perform_request(&block)
     accept = 'text/html'
     accept = 'application/activity+json, application/ld+json, application/atom+xml, ' + accept unless @unsupported_activity
 
-    @response = Request.new(:get, @url)
-                       .add_headers('Accept' => accept)
-                       .perform
+    Request.new(:get, @url).add_headers('Accept' => accept).perform(&block)
   end
 
-  def process_response(terminal = false)
-    return nil if @response.code != 200
+  def process_response(response, terminal = false)
+    return nil if response.code != 200
 
-    if @response.mime_type == 'application/atom+xml'
-      [@url, { prefetched_body: @response.to_s }, :ostatus]
-    elsif ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@response.mime_type)
-      json = body_to_json(@response.to_s)
+    if response.mime_type == 'application/atom+xml'
+      [@url, { prefetched_body: response.to_s }, :ostatus]
+    elsif ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(response.mime_type)
+      json = body_to_json(response.to_s)
       if supported_context?(json) && json['type'] == 'Person' && json['inbox'].present?
-        [json['id'], { prefetched_body: @response.to_s, id: true }, :activitypub]
+        [json['id'], { prefetched_body: response.to_s, id: true }, :activitypub]
       elsif supported_context?(json) && json['type'] == 'Note'
-        [json['id'], { prefetched_body: @response.to_s, id: true }, :activitypub]
+        [json['id'], { prefetched_body: response.to_s, id: true }, :activitypub]
       else
         @unsupported_activity = true
         nil
       end
-    elsif @response['Link'] && !terminal && link_header.find_link(%w(rel alternate))
-      process_headers
-    elsif @response.mime_type == 'text/html' && !terminal
-      process_html
+    elsif !terminal
+      link_header = response['Link'] && parse_link_header(response)
+
+      if link_header&.find_link(%w(rel alternate))
+        process_link_headers(link_header)
+      elsif response.mime_type == 'text/html'
+        process_html(response)
+      end
     end
   end
 
-  def process_html
-    page = Nokogiri::HTML(@response.to_s)
+  def process_html(response)
+    page = Nokogiri::HTML(response.to_s)
 
     json_link = page.xpath('//link[@rel="alternate"]').find { |link| ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(link['type']) }
     atom_link = page.xpath('//link[@rel="alternate"]').find { |link| link['type'] == 'application/atom+xml' }
@@ -71,7 +72,7 @@ class FetchAtomService < BaseService
     result
   end
 
-  def process_headers
+  def process_link_headers(link_header)
     json_link = link_header.find_link(%w(rel alternate), %w(type application/activity+json)) || link_header.find_link(%w(rel alternate), ['type', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'])
     atom_link = link_header.find_link(%w(rel alternate), %w(type application/atom+xml))
 
@@ -81,7 +82,7 @@ class FetchAtomService < BaseService
     result
   end
 
-  def link_header
-    @link_header ||= LinkHeader.parse(@response['Link'].is_a?(Array) ? @response['Link'].first : @response['Link'])
+  def parse_link_header(response)
+    LinkHeader.parse(response['Link'].is_a?(Array) ? response['Link'].first : response['Link'])
   end
 end
index 8f252e64c68c857ac28b1b4bce6f24818910ae56..26deb5ecc45a71a874f2c6c28cda0bf33b9e3bf7 100644 (file)
@@ -36,15 +36,24 @@ class FetchLinkCardService < BaseService
 
   def process_url
     @card ||= PreviewCard.new(url: @url)
-    res     = Request.new(:head, @url).perform
 
-    return if res.code != 405 && (res.code != 200 || res.mime_type != 'text/html')
+    failed = Request.new(:head, @url).perform do |res|
+      res.code != 405 && (res.code != 200 || res.mime_type != 'text/html')
+    end
 
-    @response = Request.new(:get, @url).perform
+    return if failed
 
-    return if @response.code != 200 || @response.mime_type != 'text/html'
+    Request.new(:get, @url).perform do |res|
+      if res.code == 200 && res.mime_type == 'text/html'
+        @html = res.to_s
+        @html_charset = res.charset
+      else
+        @html = nil
+        @html_charset = nil
+      end
+    end
 
-    @html = @response.to_s
+    return if @html.nil?
 
     attempt_oembed || attempt_opengraph
   end
@@ -118,7 +127,7 @@ class FetchLinkCardService < BaseService
     detector = CharlockHolmes::EncodingDetector.new
     detector.strip_tags = true
 
-    guess = detector.detect(@html, @response.charset)
+    guess = detector.detect(@html, @html_charset)
     page  = Nokogiri::HTML(@html, nil, guess&.fetch(:encoding, nil))
 
     if meta_property(page, 'twitter:player')
index fd6d3060564554810958f82cceba2f9331611a68..034821dc07954e246af30e2de4907c73712bec27 100644 (file)
@@ -179,11 +179,10 @@ class ResolveAccountService < BaseService
   def atom_body
     return @atom_body if defined?(@atom_body)
 
-    response = Request.new(:get, atom_url).perform
-
-    raise Mastodon::UnexpectedResponseError, response unless response.code == 200
-
-    @atom_body = response.to_s
+    @atom_body = Request.new(:get, atom_url).perform do |response|
+      raise Mastodon::UnexpectedResponseError, response unless response.code == 200
+      response.to_s
+    end
   end
 
   def actor_json
index fabba8a3e43d9235048c54ce34cf388ba89be671..3419043e5668fd5edf67958558f08ba59f7b2f54 100644 (file)
@@ -12,11 +12,9 @@ class SendInteractionService < BaseService
 
     return if !target_account.ostatus? || block_notification?
 
-    delivery = build_request.perform
-
-    raise Mastodon::UnexpectedResponseError, delivery unless delivery.code > 199 && delivery.code < 300
-
-    delivery.connection&.close
+    build_request.perform do |delivery|
+      raise Mastodon::UnexpectedResponseError, delivery unless delivery.code > 199 && delivery.code < 300
+    end
   end
 
   private
index 2f725e2ec1d118661991064b8b9f03d1eb70dc72..2893b5410358337dfff1cd854ba514f727fe8765 100644 (file)
@@ -6,21 +6,21 @@ class SubscribeService < BaseService
 
     @account        = account
     @account.secret = SecureRandom.hex
-    @response       = build_request.perform
-
-    if response_failed_permanently?
-      # We're not allowed to subscribe. Fail and move on.
-      @account.secret = ''
-      @account.save!
-    elsif response_successful?
-      # The subscription will be confirmed asynchronously.
-      @account.save!
-    else
-      # The response was either a 429 rate limit, or a 5xx error.
-      # We need to retry at a later time. Fail loudly!
-      raise Mastodon::UnexpectedResponseError, @response
+
+    build_request.perform do |response|
+      if response_failed_permanently? response
+        # We're not allowed to subscribe. Fail and move on.
+        @account.secret = ''
+        @account.save!
+      elsif response_successful? response
+        # The subscription will be confirmed asynchronously.
+        @account.save!
+      else
+        # The response was either a 429 rate limit, or a 5xx error.
+        # We need to retry at a later time. Fail loudly!
+        raise Mastodon::UnexpectedResponseError, response
+      end
     end
-    @response.connection&.close
   end
 
   private
@@ -47,12 +47,12 @@ class SubscribeService < BaseService
   end
 
   # Any response in the 3xx or 4xx range, except for 429 (rate limit)
-  def response_failed_permanently?
-    (@response.status.redirect? || @response.status.client_error?) && !@response.status.too_many_requests?
+  def response_failed_permanently?(response)
+    (response.status.redirect? || response.status.client_error?) && !response.status.too_many_requests?
   end
 
   # Any response in the 2xx range
-  def response_successful?
-    @response.status.success?
+  def response_successful?(response)
+    response.status.success?
   end
 end
index 01f5c6b7ac02198d1245ed5dd463d4c79ed912ab..95c1fb4fc01acf28e241d1cd6903734d880880c3 100644 (file)
@@ -7,10 +7,9 @@ class UnsubscribeService < BaseService
     @account = account
 
     begin
-      @response = build_request.perform
-
-      Rails.logger.debug "PuSH unsubscribe for #{@account.acct} failed: #{@response.status}" unless @response.status.success?
-      @response.connection&.close
+      build_request.perform do |response|
+        Rails.logger.debug "PuSH unsubscribe for #{@account.acct} failed: #{response.status}" unless response.status.success?
+      end
     rescue HTTP::Error, OpenSSL::SSL::SSLError => e
       Rails.logger.debug "PuSH unsubscribe for #{@account.acct} failed: #{e}"
     end
index 4763856ac36dc3bd2e567fcad07bde8aa8653258..e6cfd0d07e9c8ac26d52f62306276c060b1fa62f 100644 (file)
@@ -12,11 +12,10 @@ class ActivityPub::DeliveryWorker
     @source_account = Account.find(source_account_id)
     @inbox_url      = inbox_url
 
-    perform_request
+    perform_request do |response|
+      raise Mastodon::UnexpectedResponseError, response unless response_successful? response
+    end
 
-    raise Mastodon::UnexpectedResponseError, @response unless response_successful?
-
-    @response.connection&.close
     failure_tracker.track_success!
   rescue => e
     failure_tracker.track_failure!
@@ -31,12 +30,12 @@ class ActivityPub::DeliveryWorker
     request.add_headers(HEADERS)
   end
 
-  def perform_request
-    @response = build_request.perform
+  def perform_request(&block)
+    build_request.perform(&block)
   end
 
-  def response_successful?
-    @response.code > 199 && @response.code < 300
+  def response_successful?(response)
+    response.code > 199 && response.code < 300
   end
 
   def failure_tracker
index e1ccfb99c6f377f580363ffdfb08415b491fc163..cc2d1225bf0c6234fa3f47fe75437e1743750d53 100644 (file)
@@ -21,8 +21,8 @@ class Pubsubhubbub::ConfirmationWorker
   def process_confirmation
     prepare_subscription
 
-    confirm_callback
-    logger.debug "Confirming PuSH subscription for #{subscription.callback_url} with challenge #{challenge}: #{callback_response_body}"
+    callback_get_with_params
+    logger.debug "Confirming PuSH subscription for #{subscription.callback_url} with challenge #{challenge}: #{@callback_response_body}"
 
     update_subscription
   end
@@ -44,7 +44,7 @@ class Pubsubhubbub::ConfirmationWorker
   end
 
   def response_matches_challenge?
-    callback_response_body == challenge
+    @callback_response_body == challenge
   end
 
   def subscribing?
@@ -55,16 +55,10 @@ class Pubsubhubbub::ConfirmationWorker
     mode == 'unsubscribe'
   end
 
-  def confirm_callback
-    @_confirm_callback ||= callback_get_with_params
-  end
-
   def callback_get_with_params
-    Request.new(:get, subscription.callback_url, params: callback_params).perform
-  end
-
-  def callback_response_body
-    confirm_callback.body.to_s
+    Request.new(:get, subscription.callback_url, params: callback_params).perform do |response|
+      @callback_response_body = response.body.to_s
+    end
   end
 
   def callback_params
index a9174edd23d4dd7c4da62cb84d7d4e6f32c6cbc6..619bfa48aad215c36f357114e608c62d8cbee336 100644 (file)
@@ -23,22 +23,17 @@ class Pubsubhubbub::DeliveryWorker
   private
 
   def process_delivery
-    payload_delivery
+    callback_post_payload do |payload_delivery|
+      raise Mastodon::UnexpectedResponseError, payload_delivery unless response_successful? payload_delivery
+    end
 
-    raise Mastodon::UnexpectedResponseError, payload_delivery unless response_successful?
-
-    payload_delivery.connection&.close
     subscription.touch(:last_successful_delivery_at)
   end
 
-  def payload_delivery
-    @_payload_delivery ||= callback_post_payload
-  end
-
-  def callback_post_payload
+  def callback_post_payload(&block)
     request = Request.new(:post, subscription.callback_url, body: payload)
     request.add_headers(headers)
-    request.perform
+    request.perform(&block)
   end
 
   def blocked_domain?
@@ -80,7 +75,7 @@ class Pubsubhubbub::DeliveryWorker
     OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), subscription.secret, payload)
   end
 
-  def response_successful?
+  def response_successful?(payload_delivery)
     payload_delivery.code > 199 && payload_delivery.code < 300
   end
 end
index cf32b149545ac1d57900fcb00f777179b5a28b7a..0972e43675fafc58d18bad06e772b82bf4ced718 100644 (file)
@@ -777,7 +777,7 @@ namespace :mastodon do
         progress_bar.increment
 
         begin
-          res = Request.new(:head, account.uri).perform
+          code = Request.new(:head, account.uri).perform(&:code)
         rescue StandardError
           # This could happen due to network timeout, DNS timeout, wrong SSL cert, etc,
           # which should probably not lead to perceiving the account as deleted, so
@@ -785,7 +785,7 @@ namespace :mastodon do
           next
         end
 
-        if [404, 410].include?(res.code)
+        if [404, 410].include?(code)
           if options[:force]
             SuspendAccountService.new.call(account)
             account.destroy
index 5da357c55634319a745b5f55c561ef98ef5b1001..4d6b20dd53bdc8880b9ed40874d1c27632b5c1c6 100644 (file)
@@ -39,12 +39,10 @@ describe Request do
 
   describe '#perform' do
     context 'with valid host' do
-      before do
-        stub_request(:get, 'http://example.com')
-        subject.perform
-      end
+      before { stub_request(:get, 'http://example.com') }
 
       it 'executes a HTTP request' do
+        expect { |block| subject.perform &block }.to yield_control
         expect(a_request(:get, 'http://example.com')).to have_been_made.once
       end
 
@@ -52,12 +50,20 @@ describe Request do
         allow(Addrinfo).to receive(:foreach).with('example.com', nil, nil, :SOCK_STREAM)
                                             .and_yield(Addrinfo.new(["AF_INET", 0, "example.com", "0.0.0.0"], :PF_INET, :SOCK_STREAM))
                                             .and_yield(Addrinfo.new(["AF_INET6", 0, "example.com", "2001:4860:4860::8844"], :PF_INET6, :SOCK_STREAM))
+
+        expect { |block| subject.perform &block }.to yield_control
         expect(a_request(:get, 'http://example.com')).to have_been_made.once
       end
 
       it 'sets headers' do
+        expect { |block| subject.perform &block }.to yield_control
         expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made
       end
+
+      it 'closes underlaying connection' do
+        expect_any_instance_of(HTTP::Client).to receive(:close)
+        expect { |block| subject.perform &block }.to yield_control
+      end
     end
 
     context 'with private host' do