-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Java::JavaLang::StackOverflowError with gsub(RegExp, Hash) #4001
Comments
Huh, weird one. I would have thought the block version would be the one with a problem. |
The stack overflow output does not give me a lot to go on...and weirdly it doesn't contain gsub at all. You say heavy concurrent load...but I wouldn't expect concurrency to play here unless something is modifying the Hash you pass in (it's possible to get cycles in a Hash's internal graph if modified concurrently). But even for that I'd expect to see a stuck thread, not a stack overflow. I wonder if you can reproduce this error, or see some oddity in the resulting escaped text, using a smaller reproduction case that just tries to hit I'll eyeball the code in the meantime and see if I can find anything obvious. |
It appears that switching to the above workarounds just delays the StackOverflow condition. We also increased the stack size to 3m. It now results in a Stack Overflow on the We are actively working on trying to create standalone code to reproduce the problem. |
We replaced the 2 above patches with following code and are able to workaround the above error: require 'active_support/core_ext/string/output_safety'
class ERB
module Util
def html_escape(s)
s = s.to_s
if s.html_safe?
s
else
s = s.dup if s.frozen?
s.gsub!('&', '&')
s.gsub!('<', '<')
s.gsub!('>', '>')
s.gsub!('"', '"')
s.gsub!("'", ''')
s.html_safe
end
end
# Aliasing twice issues a warning "discarding old...". Remove first to avoid it.
remove_method(:h)
alias h html_escape
module_function :h
singleton_class.send(:remove_method, :html_escape)
module_function :html_escape
end
end Rails.application.config.after_initialize do |app|
Haml::Helpers
module Haml::Helpers
def html_escape_without_haml_xss(text)
text = text.to_s
text = text.dup if text.frozen?
text.gsub!('&', '&')
text.gsub!('<', '<')
text.gsub!('>', '>')
text.gsub!('"', '"')
text.gsub!("'", ''')
text
end
def html_escape_with_haml_xss(text)
str = text.to_s
return text if str.html_safe?
Haml::Util.html_safe(html_escape_without_haml_xss(str))
end
def html_escape(text)
str = text.to_s
return text if str.html_safe?
Haml::Util.html_safe(html_escape_without_haml_xss(str))
end
end
end As a result we have been able to progress beyond the Stack Overflow Error. Also, there are several other gsub calls in rails and haml that use a block as the second parameter and none of them are failing at the moment. We are now seeing It appears that haml is using coffee-script to invoke the Java Script compiler, but the coffee-script gem is not thread-safe: https://github.com/rails/ruby-coffee-script/blob/master/lib/coffee_script.rb#L50 Our front-end developers are currently removing all remaining JavaScript from all haml pages to avoid the runtime JavaScript compilation. Once these changes have been completed we will remove the 2 patches above and see if they are still needed. |
Update: After removing the JavaScript from all haml pages the above context error has not re-occurred in the test environment. As soon as the test environment is available again for testing we will update this issue on what happens after removing the above |
@headius we can confirm that even with the changes to remove embedded JavaScript that the Stack Overflow problem does re-occur without the above Wish we could have somehow come up with a standalone test script for this scenario. Since we have a workaround, we are proceeding with the production upgrade from JRuby 1.7 to JRuby 9.1.2.0. 🎉 |
I am also having this issue with: I can reproduce it in just over a minute by running this code:
|
Well done @mattm404 🎉 I can also confirm that when running in a rails console, with the above patches applied, the script ran fine. |
@mattm404 @reidmorrison I have looped running this repro on master for many minutes now with no crash. We did fix an overflow do to constant caching since 9.1.2.0 came out. http://ci.jruby.org will have a nightly. Can you see if this is still failing now? I will try 91.2.0 next to make sure it does fail on my machine (MacOS, Java 8u92 for my env). |
@mattm404 After having written that I realize I have no site.js file. I take it that is not empty? If so can I get a copy. I also ran this outside of Rails. So heh...maybe my WFM is a bit premature (although I would like you to try a nightly still). |
@reidmorrison @mattm404 yeah I can confirm this only runs a short time on 9.1.2.0. So at least @mattm404 reproduction seems to be fixed. I will close but please reopen if your larger script is still crashing. |
Moving from JRuby 1.7.23 to JRuby 9.1.2.0 causes
gsub(RegExp, Hash)
to fail with a Java::JavaLang::StackOverflowError. The code works fine for several minutes and only fails after significant concurrent load.Environment
Applicable gems:
Expected Behavior
Under MRI and prior to JRuby optimization of the code, the
html_escape
completes successfully in both cases.Works without issue currently in production on JRuby 1.7.23, under high concurrent load.
Actual Behavior
Workaround
When patching the above code with an alternative call to gsub it works without issue:
So far we have not been able to get a standalone reproducible example of this failure.
The text was updated successfully, but these errors were encountered: