diff options
author | Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp> | 2018-03-24 20:49:54 +0900 |
---|---|---|
committer | Eugen Rochko <eugen@zeonfederated.com> | 2018-03-24 12:49:54 +0100 |
commit | 54b273bf993888cd079113dd588cb7a90228b93b (patch) | |
tree | d6437a702618c9108ffad35a540a222a053b2c7d /app | |
parent | 4e71b104e6d5f02069120c7a56b26888c6f0fef5 (diff) |
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.
Diffstat (limited to 'app')
-rw-r--r-- | app/helpers/jsonld_helper.rb | 6 | ||||
-rw-r--r-- | app/lib/provider_discovery.rb | 17 | ||||
-rw-r--r-- | app/lib/request.rb | 16 | ||||
-rw-r--r-- | app/models/concerns/remotable.rb | 34 | ||||
-rw-r--r-- | app/services/fetch_atom_service.rb | 47 | ||||
-rw-r--r-- | app/services/fetch_link_card_service.rb | 21 | ||||
-rw-r--r-- | app/services/resolve_account_service.rb | 9 | ||||
-rw-r--r-- | app/services/send_interaction_service.rb | 8 | ||||
-rw-r--r-- | app/services/subscribe_service.rb | 36 | ||||
-rw-r--r-- | app/services/unsubscribe_service.rb | 7 | ||||
-rw-r--r-- | app/workers/activitypub/delivery_worker.rb | 15 | ||||
-rw-r--r-- | app/workers/pubsubhubbub/confirmation_worker.rb | 18 | ||||
-rw-r--r-- | app/workers/pubsubhubbub/delivery_worker.rb | 17 |
13 files changed, 126 insertions, 125 deletions
diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 9530ad9f30f..957a2cbc982 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 5732e4fcb43..bbd3a2d43ef 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 298fb9528fa..8a127c65f22 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 69685ec83af..0f18c5d9637 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 - - 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 diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb index c0785984509..48ad5dcd373 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 8f252e64c68..26deb5ecc45 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 fd6d3060564..034821dc079 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 fabba8a3e43..3419043e566 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 2f725e2ec1d..2893b541035 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 01f5c6b7ac0..95c1fb4fc01 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 4763856ac36..e6cfd0d07e9 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 e1ccfb99c6f..cc2d1225bf0 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 a9174edd23d..619bfa48aad 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 |