some refactoring

app/controllers/submissions_controller.rb

@@ -5,7 +5,6 @@ class SubmissionsController < ApplicationController
   include SubmissionParameters
   include SubmissionScoring
 
-  around_action :with_server_sent_events, only: :run
   before_action :set_submission, only: [:download_file, :render_file, :run, :score, :show, :statistics, :stop, :test]
   before_action :set_docker_client, only: [:run, :test]
   before_action :set_files, only: [:download_file, :render_file, :show]
@@ -47,25 +46,27 @@ class SubmissionsController < ApplicationController
   end
 
   def run
-    container_id = nil
+    with_server_sent_events do |server_sent_event|
+      container_info_sent = false
       stderr = ''
       output = @docker_client.execute_run_command(@submission, params[:filename]) do |stream, chunk|
-      unless container_id
+        unless container_info_sent
-        container_id = @docker_client.container_id
+          server_sent_event.write({id: @docker_client.container.try(:id), port_bindings: @docker_client.container.try(:port_bindings)}, event: 'info')
-        @server_sent_event.write({id: container_id, ports: @docker_client.assigned_ports}, event: 'info')
+          container_info_sent = true
         end
-      @server_sent_event.write({stream => chunk}, event: 'output')
+        server_sent_event.write({stream => chunk}, event: 'output')
         stderr += chunk if stream == :stderr
       end
-    @server_sent_event.write(output, event: 'status')
+      server_sent_event.write(output, event: 'status')
       if stderr.present?
         if hint = Whistleblower.new(execution_environment: @submission.execution_environment).generate_hint(stderr)
-        @server_sent_event.write(hint, event: 'hint')
+          server_sent_event.write(hint, event: 'hint')
         else
           store_error(stderr)
         end
       end
     end
+  end
 
   def score
     render(json: score_submission(@submission))
@@ -114,7 +115,7 @@ class SubmissionsController < ApplicationController
   end
 
   def store_error(stderr)
-    ::Error.create(execution_environment_id: @submission.exercise.execution_environment_id, message: stderr)
+    ::Error.create(execution_environment_id: @submission.execution_environment.id, message: stderr)
   end
   private :store_error
 
@@ -125,16 +126,16 @@ class SubmissionsController < ApplicationController
 
   def with_server_sent_events
     response.headers['Content-Type'] = 'text/event-stream'
-    @server_sent_event = SSE.new(response.stream)
+    server_sent_event = SSE.new(response.stream)
-    @server_sent_event.write(nil, event: 'start')
+    server_sent_event.write(nil, event: 'start')
-    yield
+    yield(server_sent_event) if block_given?
-    @server_sent_event.write({code: 200}, event: 'close')
+    server_sent_event.write({code: 200}, event: 'close')
   rescue => exception
     logger.error(exception.message)
     logger.error(exception.backtrace.join("\n"))
-    @server_sent_event.write({code: 500}, event: 'close')
+    server_sent_event.write({code: 500}, event: 'close')
   ensure
-    @server_sent_event.close
+    server_sent_event.close
   end
   private :with_server_sent_events
 end

lib/docker_client.rb

@@ -7,8 +7,7 @@ class DockerClient
   MINIMUM_MEMORY_LIMIT = 4
   RETRY_COUNT = 2
 
-  attr_reader :assigned_ports
+  attr_reader :container
-  attr_reader :container_id
 
   def self.check_availability!
     Timeout.timeout(config[:connection_timeout]) { Docker.version }
@@ -58,8 +57,8 @@ class DockerClient
     (tries += 1) <= RETRY_COUNT ? retry : raise(error)
   end
 
-  def create_workspace(container)
+  def create_workspace_files(container, submission)
-    @submission.collect_files.each do |file|
+    submission.collect_files.each do |file|
       FileUtils.mkdir_p(File.join(self.class.local_workspace_path(container), file.path || ''))
       if file.file_type.binary?
         copy_file_to_workspace(container: container, file: file)
@@ -68,7 +67,7 @@ class DockerClient
       end
     end
   end
-  private :create_workspace
+  private :create_workspace_files
 
   def create_workspace_file(options = {})
     file = File.new(local_file_path(options), 'w')
@@ -79,31 +78,29 @@ class DockerClient
 
   def self.destroy_container(container)
     container.stop.kill
-    (container.port_bindings.try(:values) || []).each do |configuration|
+    container.port_bindings.values.each { |port| PortPool.release(port) }
-      port = configuration.first['HostPort'].to_i
-      PortPool.release(port)
-    end
     FileUtils.rm_rf(local_workspace_path(container)) if local_workspace_path(container)
     container.delete(force: true)
   end
 
   def execute_arbitrary_command(command, &block)
+    execute_command(command, nil, block)
+  end
+
+  def execute_command(command, before_execution_block, output_consuming_block)
     tries ||= 0
-    container = DockerContainerPool.get_container(@execution_environment)
+    @container = DockerContainerPool.get_container(@execution_environment)
-    @container_id = container.id
+    before_execution_block.try(:call)
-    send_command(command, container, &block)
+    send_command(command, @container, &output_consuming_block)
   rescue Excon::Errors::SocketError => error
     (tries += 1) <= RETRY_COUNT ? retry : raise(error)
   end
 
   [:run, :test].each do |cause|
     define_method("execute_#{cause}_command") do |submission, filename, &block|
-      container = DockerContainerPool.get_container(submission.execution_environment)
-      @container_id = container.id
-      @submission = submission
-      create_workspace(container)
       command = submission.execution_environment.send(:"#{cause}_command") % command_substitutions(filename)
-      send_command(command, container, &block)
+      create_workspace_files = proc { create_workspace_files(container, submission) }
+      execute_command(command, create_workspace_files, block)
     end
   end
 

lib/docker_container_mixin.rb

@@ -4,6 +4,6 @@ module DockerContainerMixin
   end
 
   def port_bindings
-    json['HostConfig']['PortBindings']
+    json['HostConfig']['PortBindings'].try(:map) { |key, value| [key.to_i, value.first['HostPort'].to_i] }.to_h
   end
 end

spec/controllers/submissions_controller_spec.rb

@@ -125,6 +125,7 @@ describe SubmissionsController do
 
   describe 'GET #run' do
     let(:filename) { submission.collect_files.detect(&:main_file?).name_with_extension }
+    let(:request) { get :run, filename: filename, id: submission.id }
 
     before(:each) do
       expect_any_instance_of(ActionController::Live::SSE).to receive(:write).at_least(3).times
@@ -133,27 +134,27 @@ describe SubmissionsController do
     context 'when no errors occur during execution' do
       before(:each) do
         expect_any_instance_of(DockerClient).to receive(:execute_run_command).with(submission, filename).and_return({})
-        get :run, filename: filename, id: submission.id
+        request
       end
 
       expect_assigns(docker_client: DockerClient)
-      expect_assigns(server_sent_event: ActionController::Live::SSE)
       expect_assigns(submission: :submission)
       expect_content_type('text/event-stream')
       expect_status(200)
     end
 
     context 'when an error occurs during execution' do
-      let(:hint) { "Your object 'main' of class 'Object' does not understand the method 'foo'." }
       let(:stderr) { "undefined method `foo' for main:Object (NoMethodError)" }
 
       before(:each) do
         expect_any_instance_of(DockerClient).to receive(:execute_run_command).with(submission, filename).and_yield(:stderr, stderr)
       end
 
-      after(:each) { get :run, filename: filename, id: submission.id }
+      after(:each) { request }
 
       context 'when the error is covered by a hint' do
+        let(:hint) { "Your object 'main' of class 'Object' does not understand the method 'foo'." }
+
         before(:each) do
           expect_any_instance_of(Whistleblower).to receive(:generate_hint).with(stderr).and_return(hint)
         end
@@ -236,4 +237,53 @@ describe SubmissionsController do
     expect_json
     expect_status(200)
   end
+
+  describe '#with_server_sent_events' do
+    let(:response) { ActionController::TestResponse.new }
+    before(:each) { allow(controller).to receive(:response).and_return(response) }
+
+    context 'when no error occurs' do
+      after(:each) { controller.send(:with_server_sent_events) }
+
+      it 'uses server-sent events' do
+        expect(ActionController::Live::SSE).to receive(:new).and_call_original
+      end
+
+      it "writes a 'start' event" do
+        allow_any_instance_of(ActionController::Live::SSE).to receive(:write)
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with(nil, event: 'start')
+      end
+
+      it "writes a 'close' event" do
+        allow_any_instance_of(ActionController::Live::SSE).to receive(:write)
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with({code: 200}, event: 'close')
+      end
+
+      it 'closes the stream' do
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:close).and_call_original
+      end
+    end
+
+    context 'when an error occurs' do
+      after(:each) { controller.send(:with_server_sent_events) { fail } }
+
+      it 'uses server-sent events' do
+        expect(ActionController::Live::SSE).to receive(:new).and_call_original
+      end
+
+      it "writes a 'start' event" do
+        allow_any_instance_of(ActionController::Live::SSE).to receive(:write)
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with(nil, event: 'start')
+      end
+
+      it "writes a 'close' event" do
+        allow_any_instance_of(ActionController::Live::SSE).to receive(:write)
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with({code: 500}, event: 'close')
+      end
+
+      it 'closes the stream' do
+        expect_any_instance_of(ActionController::Live::SSE).to receive(:close).and_call_original
+      end
+    end
+  end
 end

spec/lib/docker_client_spec.rb

@@ -118,15 +118,14 @@ describe DockerClient, docker: true do
     end
   end
 
-  describe '#create_workspace' do
+  describe '#create_workspace_files' do
     let(:container) { double }
 
     before(:each) do
-      docker_client.instance_variable_set(:@submission, submission)
       expect(container).to receive(:binds).at_least(:once).and_return(["#{workspace_path}:#{DockerClient::CONTAINER_WORKSPACE_PATH}"])
     end
 
-    after(:each) { docker_client.send(:create_workspace, container) }
+    after(:each) { docker_client.send(:create_workspace_files, container, submission) }
 
     it 'creates submission-specific directories' do
       expect(Dir).to receive(:mkdir).at_least(:once)
@@ -233,8 +232,8 @@ describe DockerClient, docker: true do
       expect(DockerContainerPool).to receive(:get_container).with(submission.execution_environment).and_call_original
     end
 
-    it 'creates the workspace' do
+    it 'creates the workspace files' do
-      expect(docker_client).to receive(:create_workspace)
+      expect(docker_client).to receive(:create_workspace_files)
     end
 
     it 'executes the run command' do
@@ -251,8 +250,8 @@ describe DockerClient, docker: true do
       expect(DockerContainerPool).to receive(:get_container).with(submission.execution_environment).and_call_original
     end
 
-    it 'creates the workspace' do
+    it 'creates the workspace files' do
-      expect(docker_client).to receive(:create_workspace)
+      expect(docker_client).to receive(:create_workspace_files)
     end
 
     it 'executes the test command' do

spec/lib/docker_container_mixin_spec.rb

@@ -1,18 +1,30 @@
 require 'rails_helper'
 
 describe DockerContainerMixin do
-  [:binds, :port_bindings].each do |method|
+  describe '#binds' do
-    describe "##{method}" do
+    let(:binds) { [] }
-      let(:data) { [] }
 
     it 'is defined for Docker::Container' do
-        expect(Docker::Container.instance_methods).to include(method)
+      expect(Docker::Container.instance_methods).to include(:binds)
     end
 
     it 'returns the correct information' do
-        expect(CONTAINER).to receive(:json).and_return('HostConfig' => {method.to_s.camelize => data})
+      expect(CONTAINER).to receive(:json).and_return('HostConfig' => {'Binds' => binds})
-        expect(CONTAINER.send(method)).to eq(data)
+      expect(CONTAINER.binds).to eq(binds)
     end
   end
+
+  describe '#port_bindings' do
+    let(:port) { 1234 }
+    let(:port_bindings) { {"#{port}/tcp" => [{'HostIp' => '', 'HostPort' => port.to_s}]} }
+
+    it 'is defined for Docker::Container' do
+      expect(Docker::Container.instance_methods).to include(:port_bindings)
+    end
+
+    it 'returns the correct information' do
+      expect(CONTAINER).to receive(:json).and_return('HostConfig' => {'PortBindings' => port_bindings})
+      expect(CONTAINER.port_bindings).to eq(port => port)
+    end
   end
 end