Support timeout for tool (#47)

* Support timeout for tool * Set Ruby test runner timeout to 4 * DRY-up the config code * Use timeout from tool * Fix Rubocop version * Fix test Co-authored-by: Jeremy Walker <jez.walker@gmail.com>
exercism · Sep 28, 2021 · 73f1979 · 73f1979
1 parent 0474803
commit 73f1979
Show file tree

Hide file tree

Showing 14 changed files with 51 additions and 49 deletions.
diff --git a/.github/workflows/rubocop.yml b/.github/workflows/rubocop.yml
@@ -20,7 +20,7 @@ jobs:
 
       - name: Install gems
         run: |
-          gem install rubocop -v 0.91.0
+          gem install rubocop -v 1.6.1
           gem install rubocop-minitest
           gem install rubocop-performance
 

diff --git a/lib/tooling_invoker/configuration.rb b/lib/tooling_invoker/configuration.rb
@@ -28,16 +28,26 @@ def jobs_dir
     end
 
     def max_memory_for_tool(tool)
-      tool_config = tools_config[tool.tr('-', '_')]
-      tool_config&.fetch("max_memory", nil) || "3GB"
+      tool_setting(tool, "max_memory", "3GB")
     end
 
     def network_for_tool(tool)
-      tool_config = tools_config[tool.tr('-', '_')]
-      tool_config&.fetch("network", nil) || "none"
+      tool_setting(tool, "network", "none")
+    end
+
+    def timeout_for_tool(tool)
+      tool_setting(tool, "timeout", 20).to_i
     end
 
     private
+    def tool_setting(tool, setting, default)
+      tool_config(tool)&.fetch(setting, default) || default
+    end
+
+    def tool_config(tool)
+      tools_config[tool.tr('-', '_')]
+    end
+
     memoize
     def tools_config
       JSON.parse(File.read(File.expand_path('../../tools.json', __dir__)))

diff --git a/lib/tooling_invoker/exec_docker.rb b/lib/tooling_invoker/exec_docker.rb
@@ -10,9 +10,6 @@ class ExecDocker
 
     def initialize(job)
       @job = job
-      @timeout_s = job.timeout_s.to_i
-      @timeout_s = 20 unless @timeout_s > 0
-
       @container_label = "exercism-#{job.id}-#{SecureRandom.hex}"
     end
 
@@ -31,7 +28,7 @@ def call
         # Run the command in a thread and timeout just
         # after the SIGKILL is sent inside ExternalCommand timeout.
         # Note whether it existed cleanly (success) or timed out
-        success = docker_thread.join(timeout_s + 1.1)
+        success = docker_thread.join(Configuration.instance.timeout_for_tool(job.tool) + 1.1)
 
         # If we get to this stage, and the thread didn't exit
         # clearly, it's still running and we need to stop it
@@ -58,7 +55,7 @@ def call
     end
 
     private
-    attr_reader :job, :container_label, :timeout_s, :pid
+    attr_reader :job, :container_label, :pid
 
     def exec_command!
       captured_stdout = []
@@ -163,6 +160,7 @@ def docker_run_command
         *job.invocation_args
       ].join(" ")
 
+      timeout_s = Configuration.instance.timeout_for_tool(job.tool)
       timeout_cmd = "/usr/bin/timeout -s SIGTERM -k 1 #{timeout_s} #{docker_cmd}"
       Exercism.env.development? ? docker_cmd : timeout_cmd
     end

diff --git a/lib/tooling_invoker/jobs/job.rb b/lib/tooling_invoker/jobs/job.rb
@@ -13,17 +13,16 @@ class Job
 
       MAX_OUTPUT_FILE_SIZE = 500 * 1024 # 500 kilobyte
 
-      attr_reader :id, :language, :exercise, :source, :container_version, :timeout_s, :exception
+      attr_reader :id, :language, :exercise, :source, :container_version, :exception
       attr_accessor :stdout, :stderr
       attr_writer :output # Used by local webserver
 
-      def initialize(id, language, exercise, source, container_version, timeout_s)
+      def initialize(id, language, exercise, source, container_version)
         @id = id
         @language = language
         @exercise = exercise
         @source = source
         @container_version = container_version
-        @timeout_s = timeout_s
 
         @status = DID_NOT_EXECUTE_STATUS
         @stdout = ""

diff --git a/lib/tooling_invoker/worker.rb b/lib/tooling_invoker/worker.rb
@@ -73,8 +73,7 @@ def check_for_job
         job_data['language'],
         job_data['exercise'],
         job_data['source'],
-        job_data['container_version'],
-        job_data['timeout']
+        job_data['container_version']
       )
     rescue RestClient::NotFound
       nil

diff --git a/test/configuration_test.rb b/test/configuration_test.rb
@@ -17,6 +17,8 @@ def test_production_defaults
       assert_equal "3GB", config.max_memory_for_tool("foobar")
       assert_equal "internal", config.network_for_tool("elixir-test-runner")
       assert_equal "none", config.network_for_tool("foobar")
+      assert_equal 20, config.timeout_for_tool("foobar")
+      assert_equal 4, config.timeout_for_tool("ruby-test-runner")
       assert_equal "production", config.image_tag
     end
 

diff --git a/test/guards_test.rb b/test/guards_test.rb
@@ -17,10 +17,12 @@ def teardown
     end
 
     def test_timeout
+      # This is the timeout that we use to test this
+      Configuration.any_instance.stubs(:timeout_for_tool).returns(1)
+
       job = Jobs::TestRunnerJob.new(
         @job_id,
-        "ruby", "bob", { 'submission_filepaths' => [] }, "v1",
-        1 # This is the timeout that we use to test this
+        "ruby", "bob", { 'submission_filepaths' => [] }, "v1"
       )
       ExecDocker.any_instance.stubs(docker_run_command: "#{__dir__}/bin/infinite_loop")
 
@@ -44,10 +46,12 @@ def test_timeout
     end
 
     def test_too_many_results
+      # This is the timeout that we use to test this
+      Configuration.any_instance.stubs(:timeout_for_tool).returns(1)
+
       job = Jobs::TestRunnerJob.new(
         @job_id,
-        "ruby", "bob", { 'submission_filepaths' => [] }, "v1",
-        1 # This is the timeout that we use to test this
+        "ruby", "bob", { 'submission_filepaths' => [] }, "v1"
       )
 
       FileUtils.mkdir_p(job.source_code_dir)
@@ -62,10 +66,12 @@ def test_too_many_results
     end
 
     def test_excessive_output
+      # Ensures this is high enough to run out of output
+      Configuration.any_instance.stubs(:timeout_for_tool).returns(1)
+
       job = Jobs::TestRunnerJob.new(
         @job_id,
-        "ruby", "bob", { 'submission_filepaths' => [] }, "v1",
-        1 # Ensures this is high enough to run out of output
+        "ruby", "bob", { 'submission_filepaths' => [] }, "v1"
       )
       ExecDocker.any_instance.stubs(docker_run_command: "#{__dir__}/bin/infinite_output")
 

diff --git a/test/jobs/analyzer_job_test.rb b/test/jobs/analyzer_job_test.rb
@@ -8,15 +8,13 @@ def test_everything_is_set_correctly
       exercise = "bob"
       source = { foo: 'bar' }
       container_version = "v3"
-      timeout = "10"
 
-      job = AnalyzerJob.new(job_id, language, exercise, source, container_version, timeout)
+      job = AnalyzerJob.new(job_id, language, exercise, source, container_version)
       assert_equal job_id, job.id
       assert_equal language, job.language
       assert_equal exercise, job.exercise
       assert_equal source, job.source
       assert_equal container_version, job.container_version
-      assert_equal timeout, job.timeout_s
 
       assert_equal "bin/run.sh", job.cmd
       assert_equal [

diff --git a/test/jobs/representer_job_test.rb b/test/jobs/representer_job_test.rb
@@ -8,15 +8,13 @@ def test_everything_is_set_correctly
       exercise = "bob"
       source = { foo: 'bar' }
       container_version = "v3"
-      timeout = "10"
 
-      job = RepresenterJob.new(job_id, language, exercise, source, container_version, timeout)
+      job = RepresenterJob.new(job_id, language, exercise, source, container_version)
       assert_equal job_id, job.id
       assert_equal language, job.language
       assert_equal exercise, job.exercise
       assert_equal source, job.source
       assert_equal container_version, job.container_version
-      assert_equal timeout, job.timeout_s
       assert_equal "bin/run.sh", job.cmd
       assert_equal [
         'bob',

diff --git a/test/jobs/test_runner_job_test.rb b/test/jobs/test_runner_job_test.rb
@@ -8,15 +8,13 @@ def test_everything_is_set_correctly
       exercise = "bob"
       source = { foo: 'bar' }
       container_version = "v3"
-      timeout = "10"
 
-      job = TestRunnerJob.new(job_id, language, exercise, source, container_version, timeout)
+      job = TestRunnerJob.new(job_id, language, exercise, source, container_version)
       assert_equal job_id, job.id
       assert_equal language, job.language
       assert_equal exercise, job.exercise
       assert_equal source, job.source
       assert_equal container_version, job.container_version
-      assert_equal timeout, job.timeout_s
       assert_equal "bin/run.sh", job.cmd
       assert_equal [
         'bob',

diff --git a/test/process_job_test.rb b/test/process_job_test.rb
@@ -8,8 +8,7 @@ def test_happy_path
         "ruby",
         "bob",
         { 'submission_filepaths' => [] },
-        "v1",
-        10
+        "v1"
       )
 
       begin
@@ -37,8 +36,7 @@ def test_failed_setup
         "ruby",
         "bob",
         {},
-        "v1",
-        10
+        "v1"
       )
 
       begin
@@ -61,8 +59,7 @@ def test_failed_invocation
         "ruby",
         "bob",
         { 'submission_filepaths' => [] },
-        "v1",
-        10
+        "v1"
       )
 
       begin

diff --git a/test/upload_metadata_test.rb b/test/upload_metadata_test.rb
@@ -12,7 +12,7 @@ def test_uploads_properly
       exercise = 'bob'
 
       id = SecureRandom.hex
-      job = Jobs::TestRunnerJob.new(id, language, exercise, nil, nil, nil)
+      job = Jobs::TestRunnerJob.new(id, language, exercise, nil, nil)
       job.expects(status: status)
       job.expects(output: output)
       job.expects(exception: exception)

diff --git a/test/worker_test.rb b/test/worker_test.rb
@@ -10,7 +10,6 @@ def setup
       @exercise = "bob"
       @source = { "foo" => 'bar' }
       @container_version = "v1"
-      @timeout = 10
     end
 
     def test_creates_network
@@ -38,17 +37,16 @@ def test_flow_for_test_runner
         language: @language,
         exercise: @exercise,
         source: @source,
-        container_version: @container_version,
-        timeout: @timeout
+        container_version: @container_version
       }
 
       status = mock
       output = mock
-      job = Jobs::Job.new(@job_id, 'ruby', 'two-fer', nil, nil, nil)
+      job = Jobs::Job.new(@job_id, 'ruby', 'two-fer', nil, nil)
       job.stubs(status: status, output: output)
 
       Jobs::TestRunnerJob.expects(:new).with(
-        @job_id, @language, @exercise, @source, @container_version, @timeout
+        @job_id, @language, @exercise, @source, @container_version
       ).returns(job)
 
       ProcessJob.expects(:call).with(job)
@@ -81,8 +79,7 @@ def test_flow_for_representer
         language: @language,
         exercise: @exercise,
         source: @source,
-        container_version: @container_version,
-        timeout: @timeout
+        container_version: @container_version
       }
 
       status = mock
@@ -91,7 +88,7 @@ def test_flow_for_representer
       job.stubs(id: @job_id, status: status, output: output)
 
       Jobs::RepresenterJob.expects(:new).with(
-        @job_id, @language, @exercise, @source, @container_version, @timeout
+        @job_id, @language, @exercise, @source, @container_version
       ).returns(job)
       ProcessJob.expects(:call).with(job)
       UploadMetadata.expects(:call).with(job)
@@ -123,8 +120,7 @@ def test_flow_for_analyzer
         language: @language,
         exercise: @exercise,
         source: @source,
-        container_version: @container_version,
-        timeout: @timeout
+        container_version: @container_version
       }
 
       status = mock
@@ -133,7 +129,7 @@ def test_flow_for_analyzer
       job.stubs(id: @job_id, status: status, output: output)
 
       Jobs::AnalyzerJob.expects(:new).with(
-        @job_id, @language, @exercise, @source, @container_version, @timeout
+        @job_id, @language, @exercise, @source, @container_version
       ).returns(job)
       ProcessJob.expects(:call).with(job)
       UploadMetadata.expects(:call).with(job)

diff --git a/tools.json b/tools.json
@@ -13,7 +13,8 @@
   },
   "ruby_test_runner": {
     "network": "none",
-    "max_memory": "1GB"
+    "max_memory": "1GB",
+    "timeout": 4
   },
   "clojurescript_test_runner": {
     "network": "none",