Merge pull request #346 from ministryofjustice/disable-connection-thr…

…ead-cache

Disable Excon sockets thread cache

app/services/prison_visits/client.rb

@@ -4,15 +4,34 @@ module PrisonVisits
   APIError    = Class.new(StandardError)
   APINotFound = Class.new(StandardError)
 
+  # This client is **NOT** thread safe. To be used with a connection pool. See below.
   class Client
     TIMEOUT = 3 # seconds
     EXCON_INSTRUMENT_NAME = 'pvb_api'.freeze
 
     def initialize(host, persistent: true)
       @host = host
+
+      # Sets `thread_safe_sockets` to `false` on the Excon connection. Setting
+      # to `false` disables Excon connection sockets cache for each thread.
+      #
+      # This cache means that there is a socket connection for every Excon
+      # connection object to the PVB API for each Thread. This is to make an
+      # Excon connection thread safe.
+      #
+      # For example if Puma runs with 4 threads, and those threads reuse a
+      # connection pool of 4 Excon connections means that effectevely there can
+      # be up to 16 live connections to the PVB API.
+      #
+      # This cache is unnecessary in our case since:
+      # - we ensure thread safety by using a connection pool
+      # - we end up opening more sockets than necessary (16 vs 4). If we only
+      #   have 4 puma threads we only need 4 sockets
+      # - the cache has a memory leak when there are short lived threads.
       @connection = Excon.new(
         host, persistent: persistent, connect_timeout: TIMEOUT,
               read_timeout: TIMEOUT, write_timeout: TIMEOUT, retry_limit: 3,
+              thread_safe_sockets: false,
               instrumentor: ActiveSupport::Notifications,
               instrumentor_name: EXCON_INSTRUMENT_NAME
       )