How should we rescue Java Throwables in the plugin threads and the pipeline thread #4064
Comments
|
I did some experiments to see how |
|
@jsvd - how did IRB rescue that java exception and not die? |
|
More weirdness: def runner5
STDERR.puts Thread.current.inspect
th = Thread.new do
begin
STDERR.puts Thread.current.inspect
raise java.lang.ArithmeticException.new
rescue Object => e
STDERR.puts 'Rescue in child thread', e.class
raise e
ensure
STDERR.puts 'Ensure in child thread'
end
end
th.join
rescue Object => e
STDERR.puts 'Rescue in main thread', e.class
ensure
STDERR.puts 'Ensure in main thread'
endthis output: Rescue in child thread is printed Maybe it is IRB doing something different |
|
For a running program same outcome: |
|
Same results when begin rescue is in the main code: |
|
Ok, it looks like we made a decision some time ago (caa7f5ad7b8d8148384be33981f7cf3880377df0) to only propagate subclasses of java.lang.Error on the main thread for However, I think it may have been too conservative. It should be an expectation of any code calling into Ruby that it could raise any exception, even checked exceptions normally not expected on that thread. This is due to Here's a proposed diff that allows all Throwable to propagate out of a child thread through the main thread: diff --git a/core/src/main/java/org/jruby/RubyThread.java b/core/src/main/java/org/jruby/RubyThread.java
index 2e39b1d..72bb924 100644
--- a/core/src/main/java/org/jruby/RubyThread.java
+++ b/core/src/main/java/org/jruby/RubyThread.java
@@ -1217,7 +1217,7 @@ public class RubyThread extends RubyObject implements ExecutionContext {
assert isCurrent();
Ruby runtime = getRuntime();
- if (abortOnException(runtime) && exception instanceof Error) {
+ if (abortOnException(runtime)) {
// re-propagate on main thread
runtime.getThreadService().getMainThread().raise(JavaUtil.convertJavaToUsableRubyObject(runtime, exception));
} else { |
|
A possible workaround, by setting the default uncaught exception handler for a given Ruby thread (untested code): native_thread = JRuby.reference(Thread.current).nativeThread
native_thread.set_uncaught_exception_handler {|th, t| Thread.main.raise t}Similar could be done in Java with a bit less magic and a bit less overhead. Edit: fixed set_uncaught_exception_handler method call. |
|
After debugging with @headius on IRC this works: Thread.abort_on_exception = true
begin
STDERR.puts Thread.current.inspect
th = Thread.new do
JRuby.reference(Thread.current).nativeThread.set_uncaught_exception_handler {|th, t| Thread.main.raise t}
begin
STDERR.puts Thread.current.inspect
raise java.lang.ArithmeticException.new
rescue Object => e
STDERR.puts 'Rescue in child thread', e.class
raise e
ensure
STDERR.puts 'Ensure in child thread'
end
end
th.join
rescue Object => e
STDERR.puts 'Rescue in main thread', e.class
ensure
STDERR.puts 'Ensure in main thread'
endgives output: |
|
Now, I don't necessarily recommend this workaround for general use, but within a controlled environment like Logstash it's less of a concern. |
|
Sure. It means that every child thread we define will have to have that extra line. |
|
I am going to send a failing spec to the jruby team - maybe we will have a fix in 1.7.23 and 9000 Should we wait until then? |
|
@guyboertje I think your proposal is good - to |
|
As a style concern should we write code like begin
do_something_sketchy
rescue StandardError => e
do_something_with(e.backtrace)
rescue Object => e
do_something_with(e.stacktrace)
endsince IIRC the interfaces are different? |
|
I am not sure why the Java Why can't we just wrap the worker(s) in a rescue block similar to @ph suggestion in #3990 but instead of just plain re-raising, we wrap that exception in a class WorkerException < Exception
attr_reader :cause
def initialize(message = nil, cause = nil)
super(message)
@cause = cause
end
def to_s
"#{super.inspect}, cause=#{@cause.inspect}"
end
end
def filterworker
LogStash::Util.set_thread_name("|worker")
begin
...
rescue Exception => e
raise(WorkerException.new(e.message, e))
ensure
...
end
end@guyboertje and your example above could be something like that: Thread.abort_on_exception = true
class WorkerException < Exception
attr_reader :cause
def initialize(message = nil, cause = nil)
super(message)
@cause = cause
end
def to_s
"#{super.inspect}, cause=#{@cause.inspect}"
end
end
begin
STDERR.puts Thread.current.inspect
th = Thread.new do
begin
STDERR.puts "Child thread #{Thread.current.inspect}"
raise java.lang.ArithmeticException.new("foo")
rescue Exception => e
STDERR.puts "Rescue in child thread #{e.inspect}"
raise WorkerException.new(e.message, e)
ensure
STDERR.puts 'Ensure in child thread'
end
end
th.join
sleep(5)
rescue Exception => e
STDERR.puts "Rescue in main thread #{e.inspect}"
ensure
STDERR.puts 'Ensure in main thread'
endAm I missing something here? |
|
@colinsurprenant - the workaround to set the uncaught... was a suggestion from headius. I dont think its necessary. Your suggestion is similar to mine and @andrewvc added to it too. This is ultimately something that should go into @jsvd PR #2386 |
|
@guyboertje ah, sorry, I missed your comment about CaughtJavaException - in any case, I'd vote for this solution. Also, note that if doing this, using |
|
@colin - rescue Object is necessary. Can you be certain that the error raised in the tests is truly a java based exception and not one that looks like it is. Looking at the jruby code and on IRC with headius a true java exception e.g. OOM, UOE or NPE etc is not converted to a RubyException. See this https://github.com/jruby/jruby/blob/1.7.22/core/src/main/java/org/jruby/RubyThread.java#L1211 and https://github.com/jruby/jruby/blob/1.7.22/core/src/main/java/org/jruby/runtime/Helpers.java#L3015 headius asked me to provide a test case showing this problem. His proposal will fix this problem. |
|
Here's the result of all exception handling combinations for both First I created this simple Java exception generator class: public class Foo {
public static void javaThrow(String className) throws Throwable {
Class clazz = Class.forName(className);
throw (Throwable)clazz.newInstance();
}
}1- thread rescued & re-raised exceptionrequire "java"
java_import "Foo"
Thread.abort_on_exception = true
begin
STDERR.puts("main thread #{Thread.current.inspect}")
th = Thread.new do
begin
STDERR.puts("rescued block child thread #{Thread.current.inspect} throwing #{ARGV[0]}")
Foo.javaThrow(ARGV[0])
rescue Exception => e
STDERR.puts("rescued block rescue in child thread #{e.inspect}")
raise(e)
ensure
STDERR.puts("rescued block ensure in child thread")
end
end
th.join
sleep(5)
rescue Exception => e
STDERR.puts("main thread rescue Exception #{e.inspect}")
rescue Object => e
STDERR.puts("main thread rescue Object #{e.inspect}")
ensure
STDERR.puts("main thread ensure")
end1.1 - java.lang.Error base exception (java.lang.OutOfMemoryError)1.2 - java.lang.Exception base exception (java.lang.ArithmeticException)2- thread unrescued exceptionrequire "java"
java_import "Foo"
Thread.abort_on_exception = true
begin
STDERR.puts("main thread #{Thread.current.inspect}")
th = Thread.new do
begin
STDERR.puts("unrescued block child thread #{Thread.current.inspect} throwing #{ARGV[0]}")
Foo.javaThrow(ARGV[0])
ensure
STDERR.puts("unrescued block ensure in child thread")
end
end
th.join
sleep(5)
rescue Object => e
STDERR.puts("main thread rescue Object #{e.inspect}")
rescue Exception => e
STDERR.puts("main thread rescue Exception #{e.inspect}")
ensure
STDERR.puts("main thread ensure")
end2.1 - java.lang.Error base exception (java.lang.OutOfMemoryError)2.2 - java.lang.Exception base exception (java.lang.ArithmeticException)conclusion
So the correct solution here IMO is to simply always wrap child thread (worker) exceptions in an exception wrapper class for re-raising. Here's a working example using a proper child thread rescue, wrap & re-raise with a require "java"
java_import "Foo"
Thread.abort_on_exception = true
class WrappedException < Exception
attr_reader :cause
def initialize(cause)
super(cause.message)
@cause = cause
end
def to_s
"#{super.inspect}, cause=#{@cause.inspect}"
end
end
begin
STDERR.puts("main thread #{Thread.current.inspect}")
th = Thread.new do
begin
STDERR.puts("rescued block child thread #{Thread.current.inspect} throwing #{ARGV[0]}")
Foo.javaThrow(ARGV[0])
rescue Exception => e
STDERR.puts("rescued block rescue in child thread #{e.inspect}")
raise(WrappedException.new(e))
ensure
STDERR.puts("rescued block ensure in child thread")
end
end
th.join
sleep(5)
rescue Exception => e
STDERR.puts("main thread rescue Exception #{e.inspect}")
rescue Object => e
STDERR.puts("main thread rescue Object #{e.inspect}")
ensure
STDERR.puts("main thread ensure")
end |
|
Thank you @colinsurprenant for your detailed investigation. I was wrong about the rescue Object. As can be seen from the above, it is necessary that we revisit all the plugin error handling blocks. This means we need to reconsider @jsvd PR #2386 and that we need to look at how the main pipeline thread handles exceptions thrown from input and output threads and not just worker threads. |
|
+1 on revisiting error handling & #2386 |
|
@colinsurprenant - @headius asked me to provide a spec/test case illustrating the problem. Initially looked at https://github.com/jruby/jruby/blob/master/spec/java_integration/exceptions/rescue_spec.rb. I see no obvious way to include a java class that throws a raw java exception. |
|
Closing this. Moved discussion to #4127 |
Background: any Java Throwable exceptions/errors thrown by JVM, JRuby extensions and JRuby are wrapped by JavaProxy and so are subclassed of Object and not Exception. This means that
rescue Exception => ewill not rescue them. Onlyrescue Object => ewill catch Java Throwable instances.This has big implications for how we log all errors and how to decide whether to raise, retry or not nothing.
We should revisit the change that @ph made #3990 and reconsider @jsvd PR #2386
The text was updated successfully, but these errors were encountered: