From 99320598093c92c7ab8e43b5558b96ebd8ecd6a4 Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Tue, 20 Aug 2024 07:53:40 -0700 Subject: [PATCH 1/4] Add a connect timeout for serving up images --- app/models/iiif_image.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/iiif_image.rb b/app/models/iiif_image.rb index a2a59d45..20004c94 100644 --- a/app/models/iiif_image.rb +++ b/app/models/iiif_image.rb @@ -19,7 +19,7 @@ def initialize(stacks_file:, transformation:, base_uri: Settings.imageserver.bas def response with_retries max_tries: 3, rescue: [HTTP::ConnectionError] do benchmark "Fetch #{image_url}" do - HTTP.use({ normalize_uri: { normalizer: lambda(&:itself) } }).get(image_url) + HTTP.timeout(connect: 15).use({ normalize_uri: { normalizer: lambda(&:itself) } }).get(image_url) end end end From d7b1fbc95bf4dd973204a3ddbf108c89bbae1986 Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Tue, 20 Aug 2024 07:54:12 -0700 Subject: [PATCH 2/4] Add a user agent so hopefully ops don't block us at the laod balancer. --- .rubocop.yml | 3 +++ app/models/iiif_image.rb | 5 ++++- app/services/iiif_metadata_service.rb | 4 +++- config/settings.yml | 2 ++ spec/models/projection_spec.rb | 12 ++++++------ spec/requests/iiif_auth_request_spec.rb | 2 +- spec/services/iiif_metadata_service_spec.rb | 4 ++-- 7 files changed, 21 insertions(+), 11 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 34b5a194..16acdda8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -64,6 +64,9 @@ Style/StringLiterals: RSpec/MultipleMemoizedHelpers: Enabled: false +RSpec/MessageChain: + Enabled: false + Layout/EmptyLinesAroundAttributeAccessor: Enabled: true Layout/SpaceAroundMethodCallOperator: diff --git a/app/models/iiif_image.rb b/app/models/iiif_image.rb index 20004c94..372623a9 100644 --- a/app/models/iiif_image.rb +++ b/app/models/iiif_image.rb @@ -19,7 +19,10 @@ def initialize(stacks_file:, transformation:, base_uri: Settings.imageserver.bas def response with_retries max_tries: 3, rescue: [HTTP::ConnectionError] do benchmark "Fetch #{image_url}" do - HTTP.timeout(connect: 15).use({ normalize_uri: { normalizer: lambda(&:itself) } }).get(image_url) + HTTP.timeout(connect: 15) + .headers(user_agent: "#{HTTP::Request::USER_AGENT} (#{Settings.user_agent})") + .use({ normalize_uri: { normalizer: lambda(&:itself) } }) + .get(image_url) end end end diff --git a/app/services/iiif_metadata_service.rb b/app/services/iiif_metadata_service.rb index 5d26c91c..cfe18211 100644 --- a/app/services/iiif_metadata_service.rb +++ b/app/services/iiif_metadata_service.rb @@ -47,7 +47,9 @@ def retrieve handle_response( # Disable url normalization as an upstream bug in addressable causes issues for `+` # https://github.com/sporkmonger/addressable/issues/386 - HTTP.use({ normalize_uri: { normalizer: lambda(&:itself) } }).get(@url) + HTTP.timeout(connect: 15) + .headers(user_agent: "#{HTTP::Request::USER_AGENT} (#{Settings.user_agent})") + .use({ normalize_uri: { normalizer: lambda(&:itself) } }).get(@url) ) end end diff --git a/config/settings.yml b/config/settings.yml index 8f6b591f..34dd3aa8 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -62,3 +62,5 @@ cors: token: default_expiry_time: <%= 1.hour %> + +user_agent: 'stacks.stanford.edu' diff --git a/spec/models/projection_spec.rb b/spec/models/projection_spec.rb index 01b3900b..608ee0cd 100644 --- a/spec/models/projection_spec.rb +++ b/spec/models/projection_spec.rb @@ -110,7 +110,7 @@ let(:options) { { size: 'max', region: 'full' } } it 'allows the user to see the full-resolution image' do - allow(HTTP).to receive(:use).and_return(http_client) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use).and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response expect(http_client).to have_received(:get).with(%r{/full/max/0/default.jpg}) @@ -121,7 +121,7 @@ let(:options) { { size: '!850,700', region: 'full' } } it 'returns original size when requested dimensions are larger' do - allow(HTTP).to receive(:use).and_return(http_client) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use).and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response expect(http_client).to have_received(:get).with(%r{/full/!800,600/0/default.jpg}) @@ -138,7 +138,7 @@ let(:options) { { size: 'max', region: 'full' } } it 'limits users to a thumbnail' do - allow(HTTP).to receive(:use) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use) .and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response @@ -150,7 +150,7 @@ let(:options) { { size: '!100,100', region: 'full' } } it 'limits users to a thumbnail' do - allow(HTTP).to receive(:use) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use) .and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response @@ -162,7 +162,7 @@ let(:options) { { size: '!800,880', region: 'full' } } it 'limits users to a thumbnail' do - allow(HTTP).to receive(:use) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use) .and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response @@ -174,7 +174,7 @@ let(:options) { { size: '100,100', region: 'square' } } it 'limits users to a thumbnail' do - allow(HTTP).to receive(:use) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use) .and_return(http_client) allow(http_client).to receive(:get).and_return(double(body: nil)) subject.response diff --git a/spec/requests/iiif_auth_request_spec.rb b/spec/requests/iiif_auth_request_spec.rb index 221fc6ae..ff1e06d3 100644 --- a/spec/requests/iiif_auth_request_spec.rb +++ b/spec/requests/iiif_auth_request_spec.rb @@ -27,7 +27,7 @@ describe "#show" do before do allow_any_instance_of(Projection).to receive(:valid?).and_return(true) - allow(HTTP).to receive(:use).and_return(http_client) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use).and_return(http_client) allow(http_client).to receive(:get).and_return(instance_double(HTTP::Response, status: 200, body: StringIO.new)) allow_any_instance_of(IiifController).to receive(:current_user).and_return(current_user) allow_any_instance_of(IiifController).to receive(:current_image).and_return(current_image) diff --git a/spec/services/iiif_metadata_service_spec.rb b/spec/services/iiif_metadata_service_spec.rb index a8a1c655..c7910c1d 100644 --- a/spec/services/iiif_metadata_service_spec.rb +++ b/spec/services/iiif_metadata_service_spec.rb @@ -22,7 +22,7 @@ let(:response) { instance_double(HTTP::Response, code: 200, body: json) } before do - allow(HTTP).to receive(:use).and_return(http_client) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use).and_return(http_client) allow(http_client).to receive(:get) .with("https://sul-imageserver-uat.stanford.edu/cantaloupe/iiif/2/#{image_server_path(druid, file_name)}/info.json") .and_return(response) @@ -57,7 +57,7 @@ let(:empty_json) { '' } let(:bad_response) { instance_double(HTTP::Response, code: 200, body: empty_json) } before do - allow(HTTP).to receive(:use) + allow(HTTP).to receive_message_chain(:timeout, :headers, :use) .and_return(http_client) allow(http_client).to receive(:get) .with("https://sul-imageserver-uat.stanford.edu/cantaloupe/iiif/2/#{image_server_path(druid, file_name)}/info.json") From 56c408b7b2040de62aa4487ef7b5457b3d217fc6 Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Mon, 19 Aug 2024 17:39:56 -0700 Subject: [PATCH 3/4] Don't hang up the whole status page if the NFS check hangs? --- config/initializers/okcomputer.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb index aa9c92e1..3f588f84 100644 --- a/config/initializers/okcomputer.rb +++ b/config/initializers/okcomputer.rb @@ -14,6 +14,20 @@ OkComputer::Registry.register 'rails_cache', OkComputer::GenericCacheCheck.new +class DirectoryCheckWithTimeout < OkComputer::DirectoryCheck + def check + Timeout.timeout(5) do + super + end + rescue Timeout::Error + mark_failure + mark_message "Timed out after 5 seconds" + end +end + +OkComputer::Registry.register 'stacks_root_dir', + DirectoryCheckWithTimeout.new(Settings.stacks.storage_root) + OkComputer::Registry.register 'stacks_mounted_dir', OkComputer::DirectoryCheck.new(Settings.stacks.storage_root) From aea02869f2fc08dc41bcb370a18e28781780ec4b Mon Sep 17 00:00:00 2001 From: Chris Beer Date: Tue, 20 Aug 2024 08:24:51 -0700 Subject: [PATCH 4/4] Add timeouts + user agents to faraday requests too --- app/models/cocina.rb | 6 +++++- app/services/metrics_service.rb | 7 +++++-- spec/controllers/object_controller_spec.rb | 17 ++++++++++------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/app/models/cocina.rb b/app/models/cocina.rb index bc6d12d0..e56c49bb 100644 --- a/app/models/cocina.rb +++ b/app/models/cocina.rb @@ -9,7 +9,11 @@ class Cocina def self.find(druid, version = :head) data = Rails.cache.fetch(metadata_cache_key(druid, version), expires_in: 10.minutes) do benchmark "Fetching public json for #{druid} version #{version}" do - response = Faraday.get(public_json_url(druid, version)) + connection = Faraday.new({ url: public_json_url(druid, version), + headers: { user_agent: Settings.user_agent }, + request: { open_timeout: 5 } }) + + response = connection.get raise Purl::Exception, response.status unless response.success? JSON.parse(response.body) diff --git a/app/services/metrics_service.rb b/app/services/metrics_service.rb index d6264d5f..0a7c7c2b 100644 --- a/app/services/metrics_service.rb +++ b/app/services/metrics_service.rb @@ -39,7 +39,10 @@ def event_data(name, properties) end def default_headers - { 'Content-Type': 'application/json' } + { + 'Content-Type': 'application/json', + 'User-Agent': Settings.user_agent + } end def post_json(url, data, headers) @@ -50,6 +53,6 @@ def post_json(url, data, headers) end def connection - @connection ||= Faraday.new(base_url) + @connection ||= Faraday.new({ url: base_url, request: { open_timeout: 5 } }) end end diff --git a/spec/controllers/object_controller_spec.rb b/spec/controllers/object_controller_spec.rb index c4cffa54..7aed97c8 100644 --- a/spec/controllers/object_controller_spec.rb +++ b/spec/controllers/object_controller_spec.rb @@ -9,13 +9,16 @@ allow_any_instance_of(StacksFile).to receive(:path).and_return(Rails.root + 'Gemfile') end + let(:connection) { instance_double(Faraday::Connection) } + describe '#show' do context 'when not logged in' do context "with an invalid druid" do let(:druid) { 'foo' } it 'returns a 404 Not Found' do - allow(Faraday).to receive(:get).with('https://purl.stanford.edu/foo.json').and_return( + allow(Faraday).to receive(:new).with(hash_including(url: 'https://purl.stanford.edu/foo.json')).and_return(connection) + allow(connection).to receive(:get).and_return( instance_double(Faraday::Response, status: 404, success?: false) ) get :show, params: { id: 'foo' } @@ -89,8 +92,8 @@ end before do - allow(Faraday).to receive(:get).with('https://purl.stanford.edu/fd063dh3727.json') - .and_return(instance_double(Faraday::Response, success?: true, body: json)) + allow(Faraday).to receive(:new).with(hash_including(url: 'https://purl.stanford.edu/fd063dh3727.json')).and_return(connection) + allow(connection).to receive(:get).and_return(instance_double(Faraday::Response, success?: true, body: json)) end it 'creates a zip' do @@ -145,8 +148,8 @@ end before do - allow(Faraday).to receive(:get).with('https://purl.stanford.edu/bb142ws0723.json') - .and_return(instance_double(Faraday::Response, success?: true, body: json)) + allow(Faraday).to receive(:new).with(hash_including(url: 'https://purl.stanford.edu/bb142ws0723.json')).and_return(connection) + allow(connection).to receive(:get).and_return(instance_double(Faraday::Response, success?: true, body: json)) end it 'redirects to login' do @@ -209,8 +212,8 @@ end before do - allow(Faraday).to receive(:get).with('https://purl.stanford.edu/bb142ws0723.json') - .and_return(instance_double(Faraday::Response, success?: true, body: json)) + allow(Faraday).to receive(:new).with(hash_including(url: 'https://purl.stanford.edu/bb142ws0723.json')).and_return(connection) + allow(connection).to receive(:get).and_return(instance_double(Faraday::Response, success?: true, body: json)) end it 'creates a zip' do