[JRuby] Padrino Thread-Safety Issue #863
Comments
|
If anyone needs a monkey patch for the time being, go ahead and stick this somewhere: require 'padrino-core/application/routing'
# @see https://github.com/padrino/padrino-framework/issues/863
module Padrino
module Routing
module ClassMethods
# Compiles the routes including deferred routes.
@@compiled_router_mutex = Mutex.new
def compiled_router
@@compiled_router_mutex.synchronize {
if deferred_routes.empty?
return router
else
deferred_routes.each { |_, routes| routes.each { |(route, dest)| route.to(dest) } }
@deferred_routes = nil
return router
end
}
end
end
end
end |
|
Thanks for bringing up this issue @sgonyea we hope to resolve this in the next release (0.10.8) |
|
Np. I should note that placing this mutex around the compiled_router method exposed another thread safety issue in the HttpRouter library. It exists in Thankfully this was changed in HttpRouter 0.11.x, but I have no idea how compatible it is with the current release of Padrino. Ideally, if routes need to be compiled (or anything), that would happen during application bootup if reload is set to false. I simply did not have enough insight into Padrino's internals to understand how to force this to happen (other than perhaps making a request immediately on app bootup). |
|
@joshbuddy Any thoughts on this? |
|
For reference, the backtrace that I got when I hit the thread-safety issue is pasted below. It bars at "Line 44", which is actually Line 42... But line 42 has "\n" chars that are getting instance_eval'd. It also barfs twice at the #[] method. This is because #[] gets re-defined dynamically and called again. Pretty much impossible to debug in a time-box. I'd say upgrade to HttpRouter 0.11.x asap, as it no longer does any of that. You'll also note the "Line 34" from my own app, in the backtrace. That's the monkey patch that I pasted above. |
|
@joshbuddy can you look at this? |
|
We've gotten around this for the moment using rack-middleware to ensure serial initialization of the routes. Its holding up well for the moment in production. Will observe for a few more days and see if this puts the issue to rest. Do let me know if you see any concerns with this approach. module Rack
# Ensures only 1 thread passes through the stack the first time (subsequent requests are allowed through concurrently).
# This is needed to work-around the thread-safety issue in http_router gem used by Padrino which results in a
# DoubleCompileError when concurrent requests occur on server startup.
#
class SerialInitializer
@@mutex = Mutex.new
@@initialized = false
def initialize(app)
logger.info "SerialInitializer: #{Thread.current} created"
@app = app
end
def call(env)
if !@@initialized
logger.info "SerialInitializer: #{Thread.current} not initialized, attempting lock"
@@mutex.synchronize do
logger.info "SerialInitializer: #{Thread.current} acquired lock!"
if !@@initialized
begin
logger.info "SerialInitializer: #{Thread.current} within lock, app not initialized, going ahead with call"
@app.call(env)
@@initialized = true
logger.info "SerialInitializer: #{Thread.current} app initialized successfully - this should only happen once!"
rescue Exception => e
logger.error "SerialInitializer: #{Thread.current} error initializing app, logging and re-raising (subsequent threads will re-attempt initialization): #{e.class} #{e.message} \n #{e.backtrace.join("\n")}"
raise
end
else
logger.info "SerialInitializer: #{Thread.current} acquired lock, but app was already initialized, calling it"
@app.call(env)
end
end
else
@app.call(env)
end
end
end
end |
|
I like your approach, it's what I attempted to setup but couldn't get it all the way there in my time-box :). How are you setting it up to use SerialInitializer? I'll be happy to try this out in my stuff. |
|
In config.ru: use SerialInitializer On Monday, August 13, 2012, Scott Gonyea wrote:
|
|
Gotcha, cool. I'll give it a shot. |
|
Hmmm, on the first request I get a stacktrace. I'm using Puma as my development web server. In the log I get: Subsequent requests work. I'll dig around to see what's up. Also, I'll try a different web server. |
Not sure about this error. However if it happened on the first request it would have been logged by How many JRuby runtimes is Puma creating? FYI I'm using Trinidad 1.3.4 with jruby-rack 1.1.4 and rack 1.3.6. -Yogi |
|
Scott, There was a bug in the SerialInitializer where the first request's response was not getting returned due to the log statement. Here's the fixed version: module Rack
# Ensures only 1 thread passes through the stack the first time (subsequent requests are allowed through concurrently).
# This is needed to work-around the thread-safety issue in http_router gem used by Padrino which results in a
# DoubleCompileError when concurrent requests occur on server startup.
#
class SerialInitializer
@@mutex = Mutex.new
@@initialized = false
def initialize(app)
logger.info "SerialInitializer: #{Thread.current} created"
@app = app
end
def call(env)
if !@@initialized
logger.info "SerialInitializer: #{Thread.current} not initialized, attempting lock"
@@mutex.synchronize do
logger.info "SerialInitializer: #{Thread.current} acquired lock!"
if !@@initialized
begin
logger.info "SerialInitializer: #{Thread.current} within lock, app not initialized, going ahead with call"
res = @app.call(env)
@@initialized = true
logger.info "SerialInitializer: #{Thread.current} app initialized successfully - this should only happen once!"
return res # <------ this was missing!
rescue Exception => e
logger.error "SerialInitializer: #{Thread.current} error initializing app, logging and re-raising (subsequent threads will re-attempt initialization): #{e.class} #{e.message} \n #{e.backtrace.join("\n")}"
raise
end
else
logger.info "SerialInitializer: #{Thread.current} acquired lock, but app was already initialized, calling it"
return @app.call(env)
end
end
else
return @app.call(env)
end
end
end
end |
|
@dariocravero this #863 (comment) is perfect, we can add this patch ;) |
|
@DAddYE, yes we could but is there really a point if we're actually upgrading to http_router 0.11? |
|
Yesterday I've checked out the Padrino branch for 0.11 and it worked fine at MRI, but in JRuby concurrency was totally broken because of I figured that if respond_to(:method)
undef :method
end
alias :method :raw_methodand it fixed a problem that said |
|
FWIW, i hit this issue last night using Ruby 1.9.3-p194 - the (fixed version of the) SerialInitializer above fixed it for me. |
|
What should we do here for a quick 0.11.0 release? I want to try to release by next weekend. Should we push these thread issues to 0.11.1, apply a quick fix? Please advise @DAddYE @dariocravero @ujifgc @achiu FWIW, this does seem like a serious issue. So this may be worth holding off the release for? /cc @skade |
|
IMHO it's something we should fix for 0.11.0, http_router 0.11.0 does fix it so I guess the efforts should be put in upgrading Padrino to it |
|
To me it's a serious-enough issue to hold off until the dependency is upgraded. I'm a semver pansy, so I hate to see minor version bumps for dependencies in a point release. I believe the other benefit is that the newer HttpRouter releases no longer do the awful eval of a big, impenetrable string. Debugging it down to the source of the issue in HttpRouter was not my fondest hour. Anyone who runs Padrino in a multi-threaded environment (as I did) will likely slam into this issue. Especially if they use JRuby, though MRI is certainly affected as well. Lucky for me, it was an internal app and I used Padrino as an interface for driving batch tasks. So it was easy for me to figure out the cause/effect. For anyone else, it'll probably be more of a frustrating phantom. |
|
Agreed, will mark as blocker. Thanks for your feedback. |
|
@netoneko @sgonyea We have upgraded padrino to http_router 0.11. Looks like that should fix this issue as well? /cc @DAddYE @dariocravero @skade thoughts? |
|
Confirmed solved! |
|
Sad to say, this is not fixed in http_router 0.11.0. I can still trigger Java::JavaLang::ArrayIndexOutOfBoundsException by launching an ab session. $ JAVA_OPTS="-Djava.security.egd=file:/dev/./urandom" trinidad --env production --port 8888 --threadsafe & rubyjit.HttpRouter::Node$$to_code_4C97EF26DD96755123BC66BB2CBBE6572F8F20CA1934056750.file(gems/http_router-0.11.0/lib/http_router/node.rb:113) rubyjit.HttpRouter::Node$$to_code_4C97EF26DD96755123BC66BB2CBBE6572F8F20CA1934056750.file(gems/http_router-0.11.0/lib/http_router/node.rb) Running the Rack::SerialInitializer code fixed the error, |
|
FYI, we've started migrating our services from JRuby to Ruby. The lack of thread-safety in ruby libs is turning out to be a larger issue So far it seems like most ruby libraries treat concurrency as an after -Yogi |
|
This will be our priority ONE. Stay tuned, I'll fix soon! |
|
@yogi - Developing on JRuby does require a heightened awareness that Thread safety issues may exist in some obscure library you're using. But I built a very thread-heavy application on JRuby and found it to be well worth the effort. Your best bet is to stay on the beaten path and to use libraries that are actively maintained. One option may be to just use less Padrino; I believe it's not all that difficult to use plain old Sinatra for your routes and to use the Padrino helpers elsewhere. |
|
I seem to be running into the same issue where padrino will crash with multiple incoming requests right after startup. The stack trace I see: |
|
Hey guys, it turns out the concurrent serialization is necessary for each mounted app. I've modified the above SerialInitializer to account for individual apps. This works for me. Would be so awesome if we could get rid of it in the next padrino release :) module Rack
# Ensures only 1 thread passes through the stack the first time (subsequent requests are allowed through concurrently).
# This is needed to work-around the thread-safety issue in http_router gem used by Padrino which results in a
# DoubleCompileError when concurrent requests occur on server startup.
#
class SerialInitializer
@@apps_initialized = {}
@@mutexes = {}
def initialize(app)
logger.info "SerialInitializer: #{Thread.current} created"
@app = app
mapping = @app.instance_variable_get("@mapping")
mapping.each do |host, path, match, app|
@@mutexes[path] = Mutex.new
@@apps_initialized[path] = false
end
end
def call(env)
path_info = env["PATH_INFO"].to_s
mapping = @app.instance_variable_get("@mapping")
matching_app = mapping.find do |routing_def|
host, path, match, app = *routing_def
matches = path_info =~ match && rest = $1
matches && (rest.empty? || rest[0] == ?/)
end
path = matching_app[1]
if !@@apps_initialized[path]
app_name = matching_app[3].to_s
logger.info "SerialInitializer: #{Thread.current} not initialized, attempting lock on #{app_name}"
@@mutexes[path].synchronize do
logger.info "SerialInitializer: #{Thread.current} acquired lock on #{app_name}!"
if !@@apps_initialized[path]
begin
logger.info "SerialInitializer: #{Thread.current} within lock on #{app_name}, app not initialized, going ahead with call"
res = @app.call(env)
@@apps_initialized[path] = true
logger.info "SerialInitializer: #{Thread.current} app #{app_name} initialized successfully - this should only happen once!"
return res
rescue Exception => e
logger.error "SerialInitializer: #{Thread.current} error initializing app #{app_name}, logging and re-raising (subsequent threads will re-attempt initialization): #{e.class} #{e.message} \n #{e.backtrace.join("\n")}"
raise
end
else
logger.info "SerialInitializer: #{Thread.current} acquired lock, but app #{app_name} was already initialized, calling it"
return @app.call(env)
end
end
else
return @app.call(env)
end
end
end
end |
|
I understand it looks meta, but it's temporary until we retire problematic |
|
looks good! |
|
Are we good to remove this patch now that we're on the new router? @namusyaka @ujifgc |
|
I'm going to confirm that in a few days. |
|
I checked on |
|
Happy days! :) On Sun, Jan 11, 2015 at 11:20 AM, Igor Bochkariov notifications@github.com
|
|
@ujifgc Great! Thank you!! |
|
I'm trying to use jruby on padrino, but it doesn't look like there is a Padrino |
|
Not yet, I plan on putting together a 0.13.0.beta1 soon though. |
I've come across a thread-safety issue in Padrino, running on JRuby (1.6.7.2). I don't know if this affects MRI or Rubinius. I run into the issue using WEBrick, Puma, and Trinidad. This issue seems to occur right after boot up. Perhaps Padrino is lazily compiling routes, and should instead do so on boot-up? That's just a guess; I have not dived deep enough into Padrino.
Anyway, if I boot up my app server and, once it's listening, I immediately run a script like the following:
Note that the '&' basically causes them to happen at nearly the same time (I know you probably know).
This blows up for me every time. If I update the script to look like:
Then the first curl will block the rest of the script, until the app server completes its request. The rest of the script then executes in parallel, with no problems.
I was able to fix this thread-safety issue by wrapping
Padrino::Routing::ClassMethods#compiled_routerin a Mutex, such that it now becomes:I don't believe this is the correct solution, so I'm not submitting this as a pull-request. I'd wager that this mutex should live somewhere deeper.
The text was updated successfully, but these errors were encountered: