Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct thread safety issue in js-routes generation #90

Merged
merged 3 commits into from
Dec 24, 2013

Conversation

alexaitken
Copy link
Contributor

The reloading of routes during generation of js routes cause failures when multiple threads are including js-routes.

Error: RuntimeError: can't add a new key into hash during iteration
   (in /Users/alex/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/js-routes-0.9.6/app/assets/javascripts/js-routes.js.erb)

I believe this comes from calling Rails.application.reload_routes! while another thread is iterating the list of routes.

I have created an example application to show the issue. https://github.com/alexaitken/js_routes_failure

I removed the call to Rails.application.reload_routes! entirely. This change passes the tests and corrects the issue in the example application.

I found that the line Rails.application.reload_routes! was added back in commit 3f3dfda , but I am not sure what that change was solving. So this change may break something, that is not in the test suite.

@le0pard
Copy link
Member

le0pard commented Dec 22, 2013

Hello, @alexaitken . How in tests used "config/routes.rb" file?

@alexaitken
Copy link
Contributor Author

When we call Rails.application.initialize! it will load the config/routes.rb file.

Without a routes.rb load you will not see the thread safety issue.

If you check out at this commit a64e69c you can see the test failure.

Does that answer your question @le0pard ?

@le0pard
Copy link
Member

le0pard commented Dec 22, 2013

Yep, this help. Thank you, @alexaitken. I just thinking, is it possible move this file inside spec directory?

@alexaitken
Copy link
Contributor Author

@le0pard Turns out you can, I just pushed up the change.

@le0pard
Copy link
Member

le0pard commented Dec 22, 2013

Thanks @alexaitken. Now all is perfect. Just need wait for @bogdan , because he is main contributor of project.

@alexaitken
Copy link
Contributor Author

@le0pard Sounds good.

bogdan added a commit that referenced this pull request Dec 24, 2013
Correct thread safety issue in js-routes generation
@bogdan bogdan merged commit e62c73c into railsware:master Dec 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants