[ARVADOS] created: c76954b90045782599660a069d01efca53d026a5
git at public.curoverse.com
git at public.curoverse.com
Mon Oct 6 13:12:13 EDT 2014
at c76954b90045782599660a069d01efca53d026a5 (commit)
commit c76954b90045782599660a069d01efca53d026a5
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Oct 6 13:10:39 2014 -0400
3782: Avoid using a huge read buffer when client requests a huge range. Add test.
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index e750b2c..3c6c9d7 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -325,7 +325,7 @@ class CollectionsController < ApplicationController
bytesleft = @opts[:maxbytes].andand.to_i || 2**16
io = IO.popen([env, 'arv-get', "#{@opts[:uuid]}/#{@opts[:file]}"], 'rb')
- while bytesleft > 0 && (buf = io.read(bytesleft)) != nil
+ while bytesleft > 0 && (buf = io.read([bytesleft, 2**16].min)) != nil
# shrink the bytesleft count, if we were given a maximum byte
# count to read
if @opts.include? :maxbytes
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index b334f9f..50577ed 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -181,4 +181,20 @@ class CollectionsControllerTest < ActionController::TestCase
show_collection({uuid: NONEXISTENT_COLLECTION, id: NONEXISTENT_COLLECTION},
:active, 404)
end
+
+ test "use a reasonable read buffer even if client requests a huge range" do
+ fakefiledata = mock
+ IO.expects(:popen).returns(fakefiledata)
+ fakefiledata.expects(:read).twice.with() do |length|
+ # Fail the test if read() is called with length>1MiB:
+ length < 2**20
+ end.returns("foo\n", nil)
+ fakefiledata.expects(:close)
+ foo_file = api_fixture('collections')['foo_file']
+ @request.headers['Range'] = 'bytes=0-4294967296/*'
+ get :show_file, {
+ uuid: foo_file['uuid'],
+ file: foo_file['manifest_text'].match(/ \d+:\d+:(\S+)/)[1]
+ }, session_for(:active)
+ end
end
commit 07f98aa421dd6b1cbe82817ca9d8540dba42f604
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Oct 6 12:40:12 2014 -0400
3782: Stub IO.pipe() with StringIO instead of stubbing content with KEEP_LOCAL_STORE.
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 3c24875..e750b2c 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -324,17 +324,18 @@ class CollectionsController < ApplicationController
env['ARVADOS_API_HOST_INSECURE'] = "true" if Rails.configuration.arvados_insecure_https
bytesleft = @opts[:maxbytes].andand.to_i || 2**16
- IO.popen([env, 'arv-get', "#{@opts[:uuid]}/#{@opts[:file]}"],
- 'rb') do |io|
- while bytesleft > 0 && (buf = io.read(bytesleft)) != nil
- # shrink the bytesleft count, if we were given a
- # maximum byte count to read
- if @opts.include? :maxbytes
- bytesleft = bytesleft - buf.length
- end
- yield buf
+ io = IO.popen([env, 'arv-get', "#{@opts[:uuid]}/#{@opts[:file]}"], 'rb')
+ while bytesleft > 0 && (buf = io.read(bytesleft)) != nil
+ # shrink the bytesleft count, if we were given a maximum byte
+ # count to read
+ if @opts.include? :maxbytes
+ bytesleft = bytesleft - buf.length
end
+ yield buf
end
+ io.close
+ # "If ios is opened by IO.popen, close sets $?."
+ # http://www.ruby-doc.org/core-2.1.3/IO.html#method-i-close
Rails.logger.warn("#{@opts[:uuid]}/#{@opts[:file]}: #{$?}") if $? != 0
end
end
diff --git a/apps/workbench/test/integration/jobs_test.rb b/apps/workbench/test/integration/jobs_test.rb
index f84ab77..34259cb 100644
--- a/apps/workbench/test/integration/jobs_test.rb
+++ b/apps/workbench/test/integration/jobs_test.rb
@@ -5,27 +5,12 @@ require 'integration_helper'
class JobsTest < ActionDispatch::IntegrationTest
- def setup
- # Set up KEEP_LOCAL_STORE with a file that satisfies
- # the log collection for job 'job_with_real_log'
- # TODO: figure out a better way to store this test data
- # (e.g. in a dummy test fixture)
- #
- ENV['KEEP_LOCAL_STORE'] ||= Dir.mktmpdir
- keepdir = ENV['KEEP_LOCAL_STORE']
- open(File.join(keepdir, 'cdd549ae79fe6640fa3d5c6261d8303c'), 'w') do |f|
- f.write("2014-01-01_12:00:01 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 1\n")
- f.write("2014-01-01_12:00:02 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 2\n")
- f.write("2014-01-01_12:00:03 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 3\n")
- end
-
- @log_viewer_max_bytes = Rails.configuration.log_viewer_max_bytes
- end
-
- def teardown
- keepdir = ENV.delete 'KEEP_LOCAL_STORE'
- FileUtils.rm_rf(keepdir) if keepdir
- Rails.configuration.log_viewer_max_bytes = @log_viewer_max_bytes
+ def fakepipe_with_log_data
+ content =
+ "2014-01-01_12:00:01 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 1\n" +
+ "2014-01-01_12:00:02 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 2\n" +
+ "2014-01-01_12:00:03 zzzzz-8i9sb-0vsrcqi7whchuil 0 log message 3\n"
+ StringIO.new content, 'r'
end
test "add job description" do
@@ -57,6 +42,8 @@ class JobsTest < ActionDispatch::IntegrationTest
Capybara.current_driver = Capybara.javascript_driver
job = api_fixture('jobs')['job_with_real_log']
+ IO.expects(:popen).returns(fakepipe_with_log_data)
+
visit page_with_token("active", "/jobs/#{job['uuid']}")
assert page.has_text? job['script_version']
@@ -67,11 +54,15 @@ class JobsTest < ActionDispatch::IntegrationTest
assert page.has_text? 'log message 1'
assert page.has_text? 'log message 2'
assert page.has_text? 'log message 3'
+ refute page.has_text? 'Showing only 100 bytes of this log'
end
test 'view partial job log' do
Capybara.current_driver = Capybara.javascript_driver
+ # This config will be restored during teardown by ../test_helper.rb:
Rails.configuration.log_viewer_max_bytes = 100
+
+ IO.expects(:popen).returns(fakepipe_with_log_data)
job = api_fixture('jobs')['job_with_real_log']
visit page_with_token("active", "/jobs/#{job['uuid']}")
commit 3e2927123ab9c6c8ab48b50ed65490f809e9e18a
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Oct 6 12:00:50 2014 -0400
3782: Restore config settings changed during tests. (copied from API server)
diff --git a/apps/workbench/test/test_helper.rb b/apps/workbench/test/test_helper.rb
index 1195798..5253676 100644
--- a/apps/workbench/test/test_helper.rb
+++ b/apps/workbench/test/test_helper.rb
@@ -36,10 +36,15 @@ class ActiveSupport::TestCase
Thread.current[:arvados_api_token] = auth['api_token']
end
- def teardown
+ teardown do
Thread.current[:arvados_api_token] = nil
Thread.current[:reader_tokens] = nil
- super
+ # Restore configuration settings changed during tests
+ $application_config.each do |k,v|
+ if k.match /^[^.]*$/
+ Rails.configuration.send (k + '='), v
+ end
+ end
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list