Vote #70861

完了

Limit SCM annotate to text files under the maximum file size for viewing

Admin Redmine さんがほぼ2年前に追加. ほぼ2年前に更新.

ステータス:

Closed

優先度:

通常

担当者:

カテゴリ:

SCM_3

対象バージョン:

1.3.0_36

開始日:

2011/10/28

期日:

進捗率:

予定工数:

Redmineorg_URL:

https://www.redmine.org/issues/9484

category_id:

version_id:

issue_org_id:

9484

author_id:

5197

assigned_to_id:

comments:

status_id:

tracker_id:

plus1:

affected_version:

closed_on:

affected_version_id:

ステータス-->[Closed]

引用

説明

After recently upgrading to Redmine 1.2.1 + patch #4905 and moving from Mongrel 1.1.5 to Passenger 3.0.9 (using Ruby 1.8.7 as packaged in Ubuntu's ruby1.8 1.8.7.249-2 package, for amd64), I started getting rogue processes which used up 100% CPU every now and then. Killing these rogue processes using "kill -SIGABRT pid" produced entries like these:

Processing RepositoriesController#annotate (for 10.182.221.1 at 2011-10-28 01:51:08) [GET] Parameters: {"action"=>"annotate", "id"=>"sources-fm", "path"=>["trunk", "xmleye-1.2.3_svn473-bpelunit_results.tar.gz"], "controller"=>"repositories"} Rendering template within layouts/base Rendering repositories/annotate ActionView::TemplateError (SIGTERM) on line #17 of app/views/repositories/annotate.rhtml: 14: 15: 16: <% line_num = 1 %> 17: <% syntax_highlight(@path, to_utf8(@annotate.content)).each_line do |line| %> 18: <% revision = @annotate.revisions[line_num-1] %> 19: 20: lib/redmine/codeset_util.rb:21:in `replace_invalid_utf8' app/helpers/repositories_helper.rb:146:in `to_utf8_internal' app/helpers/repositories_helper.rb:121:in `to_utf8' app/views/repositories/annotate.rhtml:17:in `_run_rhtml_app47views47repositories47annotate46rhtml' passenger (3.0.9) lib/phusion_passenger/rack/request_handler.rb:96:in `process_request' passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:513:in `accept_and_process_next_request' passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:274:in `main_loop' passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:321:in `start_request_handler' passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:275:in `send' passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:275:in `handle_spawn_application' passenger (3.0.9) lib/phusion_passenger/utils.rb:479:in `safe_fork' passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:270:in `handle_spawn_application' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `__send__' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:180:in `start' passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:149:in `start' passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:219:in `spawn_rails_application' passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:132:in `lookup_or_add' passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:214:in `spawn_rails_application' passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:82:in `synchronize' passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:79:in `synchronize' passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:213:in `spawn_rails_application' passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:132:in `spawn_application' passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:275:in `handle_spawn_application' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `__send__' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop' passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously' passenger (3.0.9) helper-scripts/passenger-spawn-server:99

As you can see, it was trying to show the output of calling 'svn annotate' on a medium (~18MB) tar.gz file, while trying to interpret its contents as UTF-8 text.

We already have a setting for limiting the maximum size of the text files which can be viewed (instead of forcing the download) using 'View'. I think we should use the same limit for annotating the files, and also prevent Redmine from annotating binary files (using the same heuristics as in 'View').

I have attached a patch which modifies 'annotate' so it checks that the file is really a text file under the specified size limit before trying to annotate it. Otherwise, it displays a new error message explaining why it could not be annotated.

journals

Great work!
Could you add a test such as source:tags/1.2.1/test/functional/repositories_filesystem_controller_test.rb#L115 ?
--------------------------------------------------------------------------------
Toshi MARUYAMA wrote:
> Great work!
> Could you add a test such as source:tags/1.2.1/test/functional/repositories_filesystem_controller_test.rb#L115 ?

I'm glad you liked the patch :-). Sorry, I forgot to watch the issue and didn't see your message until now. I'll write it tomorrow: it's midnight right now in Spain. Basically, I just need to test that the proper error is shown when trying to annotate a file that is too big, right?
--------------------------------------------------------------------------------
Yes. Please see source:tags/1.2.1/test/functional/repositories_git_controller_test.rb#L306
--------------------------------------------------------------------------------
I have revised the patch with a new test case for the Subversion test suite. I had to change the new code a bit as well, as it was breaking one of the functional tests for Git repositories. It seems that with the old patch, a different error message would have been shown when trying to annotate binary files.

I have tried this one with 'rake test RAILS_ENV=test' using a SQLite database: all the tests seem to pass. I haven't run the tests that require an LDAP server, though.
--------------------------------------------------------------------------------
Committed in trunk r7728 and r7729, thanks.
--------------------------------------------------------------------------------