From 54b273bf993888cd079113dd588cb7a90228b93b Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 24 Mar 2018 20:49:54 +0900 Subject: [PATCH] Close http connection in perform method of Request class (#6889) 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. --- app/helpers/jsonld_helper.rb | 6 +-- app/lib/provider_discovery.rb | 17 ++++--- app/lib/request.rb | 16 +++++-- app/models/concerns/remotable.rb | 28 +++++------ app/services/fetch_atom_service.rb | 47 ++++++++++--------- app/services/fetch_link_card_service.rb | 21 ++++++--- app/services/resolve_account_service.rb | 9 ++-- app/services/send_interaction_service.rb | 8 ++-- app/services/subscribe_service.rb | 34 +++++++------- app/services/unsubscribe_service.rb | 7 ++- app/workers/activitypub/delivery_worker.rb | 15 +++--- .../pubsubhubbub/confirmation_worker.rb | 18 +++---- app/workers/pubsubhubbub/delivery_worker.rb | 17 +++---- lib/tasks/mastodon.rake | 4 +- spec/lib/request_spec.rb | 14 ++++-- 15 files changed, 134 insertions(+), 127 deletions(-) diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 9530ad9f3..957a2cbc9 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -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) diff --git a/app/lib/provider_discovery.rb b/app/lib/provider_discovery.rb index 5732e4fcb..bbd3a2d43 100644 --- a/app/lib/provider_discovery.rb +++ b/app/lib/provider_discovery.rb @@ -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 diff --git a/app/lib/request.rb b/app/lib/request.rb index 298fb9528..8a127c65f 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -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 diff --git a/app/models/concerns/remotable.rb b/app/models/concerns/remotable.rb index 69685ec83..0f18c5d96 100644 --- a/app/models/concerns/remotable.rb +++ b/app/models/concerns/remotable.rb @@ -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 + Request.new(:get, url).perform do |response| + next if response.code != 200 - 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 - 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) - send("#{attachment_name}=", StringIO.new(response.to_s)) - send("#{attachment_name}_file_name=", basename + extname) - - self[attribute_name] = url if has_attribute?(attribute_name) + 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 diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb index c07859845..48ad5dcd3 100644 --- a/app/services/fetch_atom_service.rb +++ b/app/services/fetch_atom_service.rb @@ -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 diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 8f252e64c..26deb5ecc 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -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') diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index fd6d30605..034821dc0 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -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 diff --git a/app/services/send_interaction_service.rb b/app/services/send_interaction_service.rb index fabba8a3e..3419043e5 100644 --- a/app/services/send_interaction_service.rb +++ b/app/services/send_interaction_service.rb @@ -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 diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb index 2f725e2ec..2893b5410 100644 --- a/app/services/subscribe_service.rb +++ b/app/services/subscribe_service.rb @@ -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 diff --git a/app/services/unsubscribe_service.rb b/app/services/unsubscribe_service.rb index 01f5c6b7a..95c1fb4fc 100644 --- a/app/services/unsubscribe_service.rb +++ b/app/services/unsubscribe_service.rb @@ -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 diff --git a/app/workers/activitypub/delivery_worker.rb b/app/workers/activitypub/delivery_worker.rb index 4763856ac..e6cfd0d07 100644 --- a/app/workers/activitypub/delivery_worker.rb +++ b/app/workers/activitypub/delivery_worker.rb @@ -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 diff --git a/app/workers/pubsubhubbub/confirmation_worker.rb b/app/workers/pubsubhubbub/confirmation_worker.rb index e1ccfb99c..cc2d1225b 100644 --- a/app/workers/pubsubhubbub/confirmation_worker.rb +++ b/app/workers/pubsubhubbub/confirmation_worker.rb @@ -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 diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb index a9174edd2..619bfa48a 100644 --- a/app/workers/pubsubhubbub/delivery_worker.rb +++ b/app/workers/pubsubhubbub/delivery_worker.rb @@ -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 diff --git a/lib/tasks/mastodon.rake b/lib/tasks/mastodon.rake index cf32b1495..0972e4367 100644 --- a/lib/tasks/mastodon.rake +++ b/lib/tasks/mastodon.rake @@ -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 diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb index 5da357c55..4d6b20dd5 100644 --- a/spec/lib/request_spec.rb +++ b/spec/lib/request_spec.rb @@ -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