[rb] Release a stopped service instead of holding it until the process exits

rb/lib/selenium/webdriver/common/service_manager.rb

@@ -30,6 +30,51 @@ class ServiceManager
       SOCKET_LOCK_TIMEOUT = 45
       STOP_TIMEOUT = 20
 
+      @running = []
+      @running_mutex = Mutex.new
+      @exit_hook_pid = nil
+
+      class << self
+        #
+        # Stops every service still running in this process.
+        #
+
+        def stop_running
+          @running_mutex.synchronize {
+            claim_for_this_process
+            @running.dup
+          }.each(&:stop)
+        end
+
+        #
+        # Tracks a started service so it can be stopped at exit. One exit hook covers
+        # all of them, so a service that has been stopped is not held onto.
+        #
+
+        def track(manager)
+          @running_mutex.synchronize do
+            claim_for_this_process
+            @running << manager
+          end
+        end
+
+        def untrack(manager)
+          @running_mutex.synchronize { @running.delete(manager) }
+        end
+
+        private
+
+        # Services tracked before a fork belong to the parent, which stops them itself.
+        # The exit hook is armed here rather than at load so a child gets one of its own.
+        def claim_for_this_process
+          return if @exit_hook_pid == Process.pid
+
+          @exit_hook_pid = Process.pid
+          @running.clear
+          Platform.exit_hook { stop_running }
+        end
+      end
+
       #
       # End users should use a class method for the desired driver, rather than using this directly.
       #
@@ -50,7 +95,7 @@ def initialize(config)
       def start
         raise "already started: #{uri.inspect} #{@executable_path.inspect}" if process_running?
 
-        Platform.exit_hook { stop } # make sure we don't leave the server running
+        self.class.track(self) # make sure we don't leave the server running
 
         socket_lock.locked do
           find_free_port
@@ -69,6 +114,7 @@ def stop
         nil # noop
       ensure
         stop_process
+        self.class.untrack(self)
       end
 
       def uri

rb/sig/lib/selenium/webdriver/common/service_manager.rbs

@@ -43,6 +43,20 @@ module Selenium
 
       STOP_TIMEOUT: Integer
 
+      self.@running: Array[ServiceManager]
+
+      self.@running_mutex: Thread::Mutex
+
+      self.@exit_hook_pid: Integer?
+
+      def self.stop_running: () -> void
+
+      def self.track: (ServiceManager manager) -> void
+
+      def self.untrack: (ServiceManager manager) -> void
+
+      private def self.claim_for_this_process: () -> void
+
       def initialize: (untyped config) -> void
 
       def start: () -> untyped

rb/spec/unit/selenium/webdriver/common/service_manager_spec.rb

@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+# Licensed to the Software Freedom Conservancy (SFC) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The SFC licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+require File.expand_path('../spec_helper', __dir__)
+
+module Selenium
+  module WebDriver
+    describe ServiceManager do
+      subject(:manager) { build_manager }
+
+      let(:running) { described_class.instance_variable_get(:@running) }
+
+      # The service itself is never launched; only the bookkeeping around it is under test.
+      def build_manager(port: 4444)
+        config = instance_double(Service, executable_path: '/path/to/service', port: port,
+                                          log: nil, args: [], shutdown_supported: true)
+        described_class.new(config).tap do |service_manager|
+          allow(service_manager).to receive_messages(socket_lock: yielding_lock, find_free_port: nil,
+                                                     start_process: nil, connect_until_stable: nil,
+                                                     stop_process: nil)
+        end
+      end
+
+      def yielding_lock
+        instance_double(SocketLock).tap { |lock| allow(lock).to receive(:locked).and_yield }
+      end
+
+      # The tracking list and the pid that armed the exit hook outlive an example, so a
+      # later one would otherwise see the hook as already armed.
+      before do
+        described_class.instance_variable_set(:@running, [])
+        described_class.instance_variable_set(:@exit_hook_pid, nil)
+      end
+
+      after { described_class.stop_running }
+
+      describe '.track' do
+        it 'holds a started service so it can be stopped at exit' do
+          manager.start
+
+          expect(running).to include(manager)
+        end
+
+        it 'registers a single exit hook however many services start' do
+          allow(Platform).to receive(:exit_hook)
+
+          2.times { build_manager.start }
+
+          expect(Platform).to have_received(:exit_hook).exactly(:once)
+        end
+      end
+
+      describe '.untrack' do
+        it 'releases a service once it is stopped' do
+          manager.start
+          manager.stop
+
+          expect(running).not_to include(manager)
+        end
+      end
+
+      describe '.stop_running' do
+        it 'stops a service that is still running' do
+          manager.start
+
+          described_class.stop_running
+
+          expect(manager).to have_received(:stop_process)
+        end
+
+        it 'leaves a service inherited through a fork to the parent' do
+          manager.start
+          described_class.instance_variable_set(:@exit_hook_pid, Process.pid - 1)
+
+          described_class.stop_running
+
+          expect(manager).not_to have_received(:stop_process)
+        end
+      end
+    end
+  end
+end