From c5d01c7f4581be9b8f731f1c0b70ff28b89435be Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Sun, 6 Apr 2025 23:23:21 -0700 Subject: [PATCH 1/3] Added open timeout --- lib/experiment/persistent_http_client.rb | 2 +- lib/experiment/remote/client.rb | 11 ++++++----- lib/experiment/remote/config.rb | 9 ++++++++- spec/experiment/remote/client_spec.rb | 20 ++++++++++++++++++++ 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/lib/experiment/persistent_http_client.rb b/lib/experiment/persistent_http_client.rb index d9a572b..0802a85 100644 --- a/lib/experiment/persistent_http_client.rb +++ b/lib/experiment/persistent_http_client.rb @@ -5,7 +5,7 @@ module AmplitudeExperiment # WARNING: these connections are not safe for concurrent requests. Callers # must synchronize requests per connection. class PersistentHttpClient - DEFAULT_OPTIONS = { read_timeout: 80 }.freeze + DEFAULT_OPTIONS = { open_timeout: 60, read_timeout: 80 }.freeze class << self # url: URI / String diff --git a/lib/experiment/remote/client.rb b/lib/experiment/remote/client.rb index 89efaeb..fba95fa 100644 --- a/lib/experiment/remote/client.rb +++ b/lib/experiment/remote/client.rb @@ -89,7 +89,7 @@ def fetch_async_v2(user, &callback) # @param [User] user def fetch_internal(user) @logger.debug("[Experiment] Fetching variants for user: #{user.as_json}") - do_fetch(user, @config.fetch_timeout_millis) + do_fetch(user, @config.open_timeout_millis, @config.fetch_timeout_millis) rescue StandardError => e @logger.error("[Experiment] Fetch failed: #{e.message}") if should_retry_fetch?(e) @@ -112,7 +112,7 @@ def retry_fetch(user) @config.fetch_retries.times do sleep(delay_millis.to_f / 1000.0) begin - return do_fetch(user, @config.fetch_retry_timeout_millis) + return do_fetch(user, @config.open_timeout_millis, @config.fetch_retry_timeout_millis) rescue StandardError => e @logger.error("[Experiment] Retry failed: #{e.message}") err = e @@ -124,15 +124,16 @@ def retry_fetch(user) # @param [User] user # @param [Integer] timeout_millis - def do_fetch(user, timeout_millis) + def do_fetch(user, open_timeout_millis, fetch_timeout_millis) start_time = Time.now user_context = add_context(user) headers = { 'Authorization' => "Api-Key #{@api_key}", 'Content-Type' => 'application/json;charset=utf-8' } - read_timeout = timeout_millis.to_f / 1000 if (timeout_millis.to_f / 1000) > 0 - http = PersistentHttpClient.get(@uri, { read_timeout: read_timeout }, @api_key) + open_timeout = open_timeout_millis.to_f / 1000 if (open_timeout_millis.to_f / 1000) > 0 + read_timeout = fetch_timeout_millis.to_f / 1000 if (fetch_timeout_millis.to_f / 1000) > 0 + http = PersistentHttpClient.get(@uri, { open_timeout: open_timeout, read_timeout: read_timeout }, @api_key) request = Net::HTTP::Post.new(@uri, headers) request.body = user_context.to_json @logger.warn("[Experiment] encoded user object length #{request.body.length} cannot be cached by CDN; must be < 8KB") if request.body.length > 8000 diff --git a/lib/experiment/remote/config.rb b/lib/experiment/remote/config.rb index 373073d..3e6827f 100644 --- a/lib/experiment/remote/config.rb +++ b/lib/experiment/remote/config.rb @@ -12,6 +12,10 @@ class RemoteEvaluationConfig # @return [Boolean] the value of server url attr_accessor :server_url + # The request connection open timeout, in milliseconds, used when fetching variants triggered by calling start() or setUser(). + # @return [Integer] the value of open_timeout_millis + attr_accessor :open_timeout_millis + # The request timeout, in milliseconds, used when fetching variants triggered by calling start() or setUser(). # @return [Integer] the value of fetch_timeout_millis attr_accessor :fetch_timeout_millis @@ -40,6 +44,8 @@ class RemoteEvaluationConfig # @param [Boolean] debug Set to true to log some extra information to the console. # @param [String] server_url The server endpoint from which to request variants. + # @param [Integer] open_timeout_millis The request connection open timeout, in milliseconds, used when + # fetching variants triggered by calling start() or setUser(). # @param [Integer] fetch_timeout_millis The request timeout, in milliseconds, used when fetching variants # triggered by calling start() or setUser(). # @param [Integer] fetch_retries The number of retries to attempt before failing. @@ -49,11 +55,12 @@ class RemoteEvaluationConfig # greater than the max, the max is used for all subsequent retries. # @param [Float] fetch_retry_backoff_scalar Scales the minimum backoff exponentially. # @param [Integer] fetch_retry_timeout_millis The request timeout for retrying fetch requests. - def initialize(debug: false, server_url: DEFAULT_SERVER_URL, fetch_timeout_millis: 10_000, fetch_retries: 0, + def initialize(debug: false, server_url: DEFAULT_SERVER_URL, open_timeout_millis: 60_000, fetch_timeout_millis: 10_000, fetch_retries: 0, fetch_retry_backoff_min_millis: 500, fetch_retry_backoff_max_millis: 10_000, fetch_retry_backoff_scalar: 1.5, fetch_retry_timeout_millis: 10_000) @debug = debug @server_url = server_url + @open_timeout_millis = open_timeout_millis @fetch_timeout_millis = fetch_timeout_millis @fetch_retries = fetch_retries @fetch_retry_backoff_min_millis = fetch_retry_backoff_min_millis diff --git a/spec/experiment/remote/client_spec.rb b/spec/experiment/remote/client_spec.rb index 793744f..cc27294 100644 --- a/spec/experiment/remote/client_spec.rb +++ b/spec/experiment/remote/client_spec.rb @@ -62,6 +62,16 @@ def self.test_fetch_v2_shared(response, test_user, variant_name, debug, expected test_fetch_shared response_without_payload, test_user, variant_name, false, Variant.new(key: 'on') test_fetch_shared response_with_value_without_payload, test_user, variant_name, false, Variant.new(value: 'on') + it 'open timeout failure' do + stub_request(:post, server_url) + .to_timeout + test_user = User.new(user_id: 'test_user') + client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(open_timeout_millis: 1, fetch_retries: 1, debug: true)) + variants = nil + expect { variants = client.fetch(test_user) }.to output(/Retrying fetch/).to_stdout_from_any_process + expect(variants).to eq({}) + end + it 'fetch timeout failure' do stub_request(:post, server_url) .to_timeout @@ -146,6 +156,16 @@ def self.test_fetch_async_shared(response, test_user, variant_name, debug, expec test_fetch_v2_shared response_without_payload, test_user, variant_name, false, Variant.new(key: 'on') test_fetch_v2_shared response_with_value_without_payload, test_user, variant_name, false, Variant.new(value: 'on') + it 'fetch v2 open timeout failure' do + stub_request(:post, server_url) + .to_timeout + test_user = User.new(user_id: 'test_user') + client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(open_timeout_millis: 1, fetch_retries: 1, debug: true)) + variants = nil + expect { variants = client.fetch_v2(test_user) }.to output(/Retrying fetch/).to_stdout_from_any_process + expect(variants).to eq({}) + end + it 'fetch v2 timeout failure' do stub_request(:post, server_url) .to_timeout From aba079825638ec4f81a4218bbac798a4617fd0de Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Tue, 8 Apr 2025 09:37:46 -0700 Subject: [PATCH 2/3] rename to connect_timeout --- lib/experiment/remote/client.rb | 10 +++++----- lib/experiment/remote/config.rb | 10 +++++----- spec/experiment/remote/client_spec.rb | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/experiment/remote/client.rb b/lib/experiment/remote/client.rb index fba95fa..45d9717 100644 --- a/lib/experiment/remote/client.rb +++ b/lib/experiment/remote/client.rb @@ -89,7 +89,7 @@ def fetch_async_v2(user, &callback) # @param [User] user def fetch_internal(user) @logger.debug("[Experiment] Fetching variants for user: #{user.as_json}") - do_fetch(user, @config.open_timeout_millis, @config.fetch_timeout_millis) + do_fetch(user, @config.connect_timeout_millis, @config.fetch_timeout_millis) rescue StandardError => e @logger.error("[Experiment] Fetch failed: #{e.message}") if should_retry_fetch?(e) @@ -112,7 +112,7 @@ def retry_fetch(user) @config.fetch_retries.times do sleep(delay_millis.to_f / 1000.0) begin - return do_fetch(user, @config.open_timeout_millis, @config.fetch_retry_timeout_millis) + return do_fetch(user, @config.connect_timeout_millis, @config.fetch_retry_timeout_millis) rescue StandardError => e @logger.error("[Experiment] Retry failed: #{e.message}") err = e @@ -124,16 +124,16 @@ def retry_fetch(user) # @param [User] user # @param [Integer] timeout_millis - def do_fetch(user, open_timeout_millis, fetch_timeout_millis) + def do_fetch(user, connect_timeout_millis, fetch_timeout_millis) start_time = Time.now user_context = add_context(user) headers = { 'Authorization' => "Api-Key #{@api_key}", 'Content-Type' => 'application/json;charset=utf-8' } - open_timeout = open_timeout_millis.to_f / 1000 if (open_timeout_millis.to_f / 1000) > 0 + connect_timeout = connect_timeout_millis.to_f / 1000 if (connect_timeout_millis.to_f / 1000) > 0 read_timeout = fetch_timeout_millis.to_f / 1000 if (fetch_timeout_millis.to_f / 1000) > 0 - http = PersistentHttpClient.get(@uri, { open_timeout: open_timeout, read_timeout: read_timeout }, @api_key) + http = PersistentHttpClient.get(@uri, { open_timeout: connect_timeout, read_timeout: read_timeout }, @api_key) request = Net::HTTP::Post.new(@uri, headers) request.body = user_context.to_json @logger.warn("[Experiment] encoded user object length #{request.body.length} cannot be cached by CDN; must be < 8KB") if request.body.length > 8000 diff --git a/lib/experiment/remote/config.rb b/lib/experiment/remote/config.rb index 3e6827f..690e155 100644 --- a/lib/experiment/remote/config.rb +++ b/lib/experiment/remote/config.rb @@ -13,8 +13,8 @@ class RemoteEvaluationConfig attr_accessor :server_url # The request connection open timeout, in milliseconds, used when fetching variants triggered by calling start() or setUser(). - # @return [Integer] the value of open_timeout_millis - attr_accessor :open_timeout_millis + # @return [Integer] the value of connect_timeout_millis + attr_accessor :connect_timeout_millis # The request timeout, in milliseconds, used when fetching variants triggered by calling start() or setUser(). # @return [Integer] the value of fetch_timeout_millis @@ -44,7 +44,7 @@ class RemoteEvaluationConfig # @param [Boolean] debug Set to true to log some extra information to the console. # @param [String] server_url The server endpoint from which to request variants. - # @param [Integer] open_timeout_millis The request connection open timeout, in milliseconds, used when + # @param [Integer] connect_timeout_millis The request connection open timeout, in milliseconds, used when # fetching variants triggered by calling start() or setUser(). # @param [Integer] fetch_timeout_millis The request timeout, in milliseconds, used when fetching variants # triggered by calling start() or setUser(). @@ -55,12 +55,12 @@ class RemoteEvaluationConfig # greater than the max, the max is used for all subsequent retries. # @param [Float] fetch_retry_backoff_scalar Scales the minimum backoff exponentially. # @param [Integer] fetch_retry_timeout_millis The request timeout for retrying fetch requests. - def initialize(debug: false, server_url: DEFAULT_SERVER_URL, open_timeout_millis: 60_000, fetch_timeout_millis: 10_000, fetch_retries: 0, + def initialize(debug: false, server_url: DEFAULT_SERVER_URL, connect_timeout_millis: 60_000, fetch_timeout_millis: 10_000, fetch_retries: 0, fetch_retry_backoff_min_millis: 500, fetch_retry_backoff_max_millis: 10_000, fetch_retry_backoff_scalar: 1.5, fetch_retry_timeout_millis: 10_000) @debug = debug @server_url = server_url - @open_timeout_millis = open_timeout_millis + @connect_timeout_millis = connect_timeout_millis @fetch_timeout_millis = fetch_timeout_millis @fetch_retries = fetch_retries @fetch_retry_backoff_min_millis = fetch_retry_backoff_min_millis diff --git a/spec/experiment/remote/client_spec.rb b/spec/experiment/remote/client_spec.rb index cc27294..005066b 100644 --- a/spec/experiment/remote/client_spec.rb +++ b/spec/experiment/remote/client_spec.rb @@ -66,7 +66,7 @@ def self.test_fetch_v2_shared(response, test_user, variant_name, debug, expected stub_request(:post, server_url) .to_timeout test_user = User.new(user_id: 'test_user') - client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(open_timeout_millis: 1, fetch_retries: 1, debug: true)) + client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(connect_timeout_millis: 1, fetch_retries: 1, debug: true)) variants = nil expect { variants = client.fetch(test_user) }.to output(/Retrying fetch/).to_stdout_from_any_process expect(variants).to eq({}) @@ -160,7 +160,7 @@ def self.test_fetch_async_shared(response, test_user, variant_name, debug, expec stub_request(:post, server_url) .to_timeout test_user = User.new(user_id: 'test_user') - client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(open_timeout_millis: 1, fetch_retries: 1, debug: true)) + client = RemoteEvaluationClient.new(api_key, RemoteEvaluationConfig.new(connect_timeout_millis: 1, fetch_retries: 1, debug: true)) variants = nil expect { variants = client.fetch_v2(test_user) }.to output(/Retrying fetch/).to_stdout_from_any_process expect(variants).to eq({}) From 37d55596357869e42b72b3e873f82e1b31668985 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Tue, 8 Apr 2025 13:39:47 -0700 Subject: [PATCH 3/3] Added a comment --- lib/experiment/remote/client.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/experiment/remote/client.rb b/lib/experiment/remote/client.rb index 45d9717..18af547 100644 --- a/lib/experiment/remote/client.rb +++ b/lib/experiment/remote/client.rb @@ -123,7 +123,8 @@ def retry_fetch(user) end # @param [User] user - # @param [Integer] timeout_millis + # @param [Integer] connect_timeout_millis + # @param [Integer] fetch_timeout_millis def do_fetch(user, connect_timeout_millis, fetch_timeout_millis) start_time = Time.now user_context = add_context(user)