fix(config-manager): Fixes polling interval and blocking timeout issues (#187)

Author: rashidsp

README.md

@@ -98,7 +98,7 @@ The `sdk_key` is used to compose the outbound HTTP request to the default datafi
 You can provide an initial datafile to bootstrap the  `DataFileProjectConfig`  so that it can be used immediately. The initial datafile also serves as a fallback datafile if HTTP connection cannot be established. The initial datafile will be discarded after the first successful datafile poll.
 
 **polling_interval**
-The polling_interval is used to specify a fixed delay in seconds between consecutive HTTP requests for the datafile.
+The polling interval is used to specify a fixed delay between consecutive HTTP requests for the datafile. Valid duration is between 1 and 2592000 seconds. Default is 5 minutes.
 
 **url_template**
 A string with placeholder `{sdk_key}` can be provided so that this template along with the provided `sdk_key` is used to form the target URL.
@@ -107,7 +107,7 @@ A string with placeholder `{sdk_key}` can be provided so that this template alon
 Boolean flag used to start the `AsyncScheduler` for datafile polling if set to `True`.
 
 **blocking_timeout**
-Maximum time in seconds to block the `get_config` call until config has been initialized.
+The blocking timeout period is used to specify a maximum time to wait for initial bootstrapping. Valid blocking timeout period is between 1 and 2592000 seconds. Default is 15 seconds.
 
 You may also provide your own logger, error handler, or notification center.
 
@@ -162,11 +162,11 @@ License (MIT): [https://github.com/jnunemaker/httparty/blob/master/MIT-LICENSE](
 
 **JSON Schema Validator** [https://github.com/ruby-json-schema/json-schema](https://github.com/ruby-json-schema/json-schema)  
 Copyright © 2010-2011, Lookingglass Cyber Solutions  
-License (MIT): [https://github.com/ruby-json-schema/json-schema/blob/master/LICENSE.md](https://github.com/ruby-json-schema/json-schema/blob/master/LICENSE.md) 
+License (MIT): [https://github.com/ruby-json-schema/json-schema/blob/master/LICENSE.md](https://github.com/ruby-json-schema/json-schema/blob/master/LICENSE.md)
 
 **Murmurhash3** [https://github.com/funny-falcon/murmurhash3-ruby](https://github.com/funny-falcon/murmurhash3-ruby)  
 Copyright © 2012 Sokolov Yura 'funny-falcon'  
-License (MIT): [https://github.com/funny-falcon/murmurhash3-ruby/blob/master/LICENSE](https://github.com/funny-falcon/murmurhash3-ruby/blob/master/LICENSE) 
+License (MIT): [https://github.com/funny-falcon/murmurhash3-ruby/blob/master/LICENSE](https://github.com/funny-falcon/murmurhash3-ruby/blob/master/LICENSE)
 
 
 ### Additional Code
@@ -198,4 +198,3 @@ License (MIT): [https://github.com/rubocop-hq/rubocop/blob/master/LICENSE.txt](h
 **WebMock** [https://github.com/bblimke/webmock](https://github.com/bblimke/webmock)  
 Copyright © 2009-2010 Bartosz Blimke    
 License (MIT): [https://github.com/bblimke/webmock/blob/master/LICENSE](https://github.com/bblimke/webmock/blob/master/LICENSE)
-

lib/optimizely/config_manager/http_project_config_manager.rb

@@ -67,8 +67,8 @@ def initialize(
       @datafile_url = get_datafile_url(sdk_key, url, url_template)
       @polling_interval = nil
       polling_interval(polling_interval)
-      blocking_timeout ||= Helpers::Constants::CONFIG_MANAGER['BLOCKING_TIMEOUT']
-      @blocking_timeout = blocking_timeout
+      @blocking_timeout = nil
+      blocking_timeout(blocking_timeout)
       @last_modified = nil
       @async_scheduler = AsyncScheduler.new(method(:fetch_datafile_config), @polling_interval, auto_update, @logger)
       @async_scheduler.start! if start_by_default == true
@@ -185,18 +185,73 @@ def polling_interval(polling_interval)
       #
       # polling_interval - Time in seconds after which to update datafile.
 
-      # If polling interval is less than minimum allowed interval then set it to default update interval.
+      # If valid set given polling interval, default update interval otherwise.
 
-      if (polling_interval.is_a? Integer) && (polling_interval >= Helpers::Constants::CONFIG_MANAGER['MIN_UPDATE_INTERVAL'])
-        @polling_interval = polling_interval
+      if polling_interval.nil?
+        @logger.log(
+          Logger::DEBUG,
+          "Polling interval is not provided. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']
         return
       end
 
-      @logger.log(
-        Logger::DEBUG,
-        "Invalid update_interval #{polling_interval} provided. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']}"
-      )
-      @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']
+      unless polling_interval.is_a? Integer
+        @logger.log(
+          Logger::ERROR,
+          "Polling interval '#{polling_interval}' has invalid type. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']
+        return
+      end
+
+      unless polling_interval.between?(Helpers::Constants::CONFIG_MANAGER['MIN_SECONDS_LIMIT'], Helpers::Constants::CONFIG_MANAGER['MAX_SECONDS_LIMIT'])
+        @logger.log(
+          Logger::DEBUG,
+          "Polling interval '#{polling_interval}' has invalid range. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_UPDATE_INTERVAL']
+        return
+      end
+
+      @polling_interval = polling_interval
+    end
+
+    def blocking_timeout(blocking_timeout)
+      # Sets time in seconds to block the get_config call until config has been initialized.
+      #
+      # blocking_timeout - Time in seconds after which to update datafile.
+
+      # If valid set given timeout, default blocking_timeout otherwise.
+
+      if blocking_timeout.nil?
+        @logger.log(
+          Logger::DEBUG,
+          "Blocking timeout is not provided. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']
+        return
+      end
+
+      unless blocking_timeout.is_a? Integer
+        @logger.log(
+          Logger::ERROR,
+          "Blocking timeout '#{blocking_timeout}' has invalid type. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']
+        return
+      end
+
+      unless blocking_timeout.between?(Helpers::Constants::CONFIG_MANAGER['MIN_SECONDS_LIMIT'], Helpers::Constants::CONFIG_MANAGER['MAX_SECONDS_LIMIT'])
+        @logger.log(
+          Logger::DEBUG,
+          "Blocking timeout '#{blocking_timeout}' has invalid range. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']} seconds."
+        )
+        @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']
+        return
+      end
+
+      @blocking_timeout = blocking_timeout
     end
 
     def get_datafile_url(sdk_key, url, url_template)

lib/optimizely/helpers/constants.rb

@@ -361,13 +361,15 @@ module Constants
       }.freeze
 
       CONFIG_MANAGER = {
-        # Maximum time in seconds to block the get_config call until config has been initialized.
-        'BLOCKING_TIMEOUT' => 15,
         'DATAFILE_URL_TEMPLATE' => 'https://cdn.optimizely.com/datafiles/%s.json',
+        # Default time in seconds to block the get_config call until config has been initialized.
+        'DEFAULT_BLOCKING_TIMEOUT' => 15,
         # Default config update interval of 5 minutes
         'DEFAULT_UPDATE_INTERVAL' => 5 * 60,
-        # Minimum config update interval of 1 second
-        'MIN_UPDATE_INTERVAL' => 1,
+        # Maximum update interval or blocking timeout: 30 days
+        'MAX_SECONDS_LIMIT' => 2_592_000,
+        # Minimum update interval or blocking timeout: 1 second
+        'MIN_SECONDS_LIMIT' => 1,
         # Time in seconds before which request for datafile times out
         'REQUEST_TIMEOUT' => 10
       }.freeze

spec/config_manager/http_project_config_manager_spec.rb

@@ -224,6 +224,7 @@
         sdk_key: 'QBw9gFM8oTn7ogY9ANCC1z',
         datafile: config_body_JSON,
         polling_interval: 1,
+        blocking_timeout: 2,
         notification_center: notification_center,
         logger: spy_logger
       )
@@ -291,4 +292,80 @@
       expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Must provide at least one of sdk_key or url.')
     end
   end
+
+  describe '.polling_interval' do
+    it 'should log an error when polling_interval is nil' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: nil,
+        blocking_timeout: 5,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::DEBUG, 'Polling interval is not provided. Defaulting to 300 seconds.')
+    end
+
+    it 'should log an error when polling_interval has invalid type' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: true,
+        blocking_timeout: 5,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Polling interval 'true' has invalid type. Defaulting to 300 seconds.")
+    end
+
+    it 'should log an error when polling_interval has invalid range' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: 999_999_999_999_999_999,
+        blocking_timeout: 5,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::DEBUG, "Polling interval '999999999999999999' has invalid range. Defaulting to 300 seconds.")
+    end
+  end
+
+  describe '.blocking_timeout' do
+    it 'should log an error when blocking_timeout is nil' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: 5,
+        blocking_timeout: nil,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::DEBUG, 'Blocking timeout is not provided. Defaulting to 15 seconds.')
+    end
+
+    it 'should log an error when blocking_timeout has invalid type' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: 5,
+        blocking_timeout: true,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Blocking timeout 'true' has invalid type. Defaulting to 15 seconds.")
+    end
+
+    it 'should log an error when blocking_timeout has invalid range' do
+      Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: 'sdk_key',
+        url: nil,
+        polling_interval: 5,
+        blocking_timeout: 999_999_999_999_999_999,
+        error_handler: error_handler,
+        logger: spy_logger
+      )
+      expect(spy_logger).to have_received(:log).once.with(Logger::DEBUG, "Blocking timeout '999999999999999999' has invalid range. Defaulting to 15 seconds.")
+    end
+  end
 end