interesting, so the leak was only relevant on 9.1.7.0 (am confused since the reports starts with 9.1.12) ?

We @swiftype have 9.1.7.0 in production and that's where we've observed the leak. We've rolled back, updated the code and deployed the changed version to fix the leak. 9.1.12.0 is mentioned because it was one of the versions tested locally while trying to reproduce the issue locally in development.

@kovyrin so one theory would be we are storing the result of cause in a temporary variable in that rescue and then raising out the value of that temp. Since we are not niling out temporary variable it would leak. HOWEVER, this is unwinding the stack because we are raising so that temporary variable should no longer be in scope and freed. If I was to keep with this theory this thread basically dies here and does not unwind which would then leak...but then I think you would have lots of threads laying around. So probably not that? hmm

Ok, so there seems to be two issues here.

1. The setCause error when raising a Java exception. This appears to be due to logic in Kernel#raise that, when given a Java exception, attempts to set its cause to whatever the current Ruby "cause" exception is, if that exception was also a Java exception.
2. The leaking of memory. This may be a side effect of the first problem, since these exceptions could easily be happening from during resource finalization (ensures, etc).

The first problem is caused by two conditions in Throwable.initCause:

1. The cause may not be initialized twice. This is the source of the reported stack trace.
2. The cause for a throwable may not be set to that throwable. We do check this case.

I will fix the double-init issue. @marshalium would you be able to test a master or 9.1.13 HEAD build?

I've pushed a fix to master and jruby-9.1.13.0 branches.

@headius Thanks for looking into this! Unfortunately, I ran the script from from my original comment on the jruby-9.1.13.0 branch and am still seeing the same errors.

I dug into this a bit, and I think I discovered more of what's actually going on, but I don't know how to fix it.

I added code to RubyKernel.maybeRaiseJavaException(), right before the ex.initCause call, to log thread and object ids:

if (maybeThrowable instanceof Throwable && ex != maybeThrowable && ex.getCause() == null) {
    System.out.format("Setting cause; thread_id=%s, ex.exception=%s, ex.id=%s, ex.cause=%s\n", Thread.currentThread().getId(), ((RaiseException) ex).getException(), System.identityHashCode(ex), ex.getCause());
    ex.initCause((Throwable) maybeThrowable);
}

And what it logged showed that multiple threads are trying to set the cause on the exact same exception object:

Setting cause; thread_id=13, ex.exception=CustomError, ex.id=500827034, ex.cause=null
Setting cause; thread_id=16, ex.exception=CustomError, ex.id=500827034, ex.cause=null
Setting cause; thread_id=14, ex.exception=CustomError, ex.id=500827034, ex.cause=null
Setting cause; thread_id=17, ex.exception=CustomError, ex.id=500827034, ex.cause=null
Setting cause; thread_id=15, ex.exception=CustomError, ex.id=500827034, ex.cause=null

The check that got added didn't fix anything since this is race condition where multiple threads can see the cause as null but only one of them can win when calling initCause. Also, ex.getCause() == null was already being checked on line 904 so I believe the check added is unncessary even in single threaded situations.

The reason I was seeing this with Guava CacheLoader is that when multiple threads try to load the same key in parallel, threads after the first one block on a future. Once the loading thread sets an error on the future the waiting threads see it and raise an UncheckedExecutionException whose cause is set to the error instance from the future. When the JRuby code re-raises the cause of the UncheckedExecutionException multiple JRuby threads end up raising the exact same exception instance and so they get IllegalStateExceptions.

Here's a gist with a simpler method of reproducing the issue that does not depend on Guava: https://gist.github.com/marshalium/64e9ebb4bb4ac107eee1f5a629feee18

Ok, so this is a weird behavior of initCause that interacts badly with threads. Thanks for the extra investigation.

I think the options we have are:

1. Don't set the Java cause to the Ruby exception.
2. Set the cause, but guard it behind an "already set" check and exception handling for the concurrent case.
3. Set the cause under lock to avoid setting it twice.

I am leaning toward (1) since Java typically wants you to init the cause yourself, rather than doing it automatically. I will implement (2) or (3) for now.

Another option would be not setting cause, but adding the Ruby exception to "suppressed".

New repro gist: confirmed broken on master.
With sync fix: confirmed fixed on master.

Still lots of debate in my mind about whether to sync, set suppressed, or not set anything.

sync patch pushed to master in 1f2e040

@kares I think you added the logic to set cause. What would you think about just dropping it? Java does not automatically set a cause for you unless you pass one into the constructor, so us setting it later based on a Ruby raise -- which might happen many times for the same throwable -- seems like the wrong behavior.

I vote for removal.

right, this is logic added in 9.1 to go along with with Ruby's (Ruby) exception.cause handling b8933ee

not sure how much its used/spec-ed (probably very little if any). seemed like a good fit to match JI the way Ruby's cause works - a passed or found in error-info gets propagated.

JRuby already has some places where the RaiseException's cause is init-ed (pbly all local - single threaded), since its often hard to tell a failure not seeing the native Java cause - that was the motivation to further help reduce the pain around JI although it makes much less sense now that I think about it.

obviously did not think too far :) ... feel free to remove for 9.2 we can always re-add later if someone complains he/she is not getting its Java cause set on a throwable raise from .rb land.

I will remove it and we can discuss further whether we want to restore this or something like it for 9.2.

My remaining thoughts on it:

Java only allows setting cause at construction time, or exactly once after that. Ruby sets it to $!foryouwhenraisinganexceptionifthatexceptionhasnocauseand$! is not that exception.

In Java, cause is never automatically set for you.

If someone is re-raising a Java exception in Ruby, they probably don't want to attach a cause (which might be the source of leaks, if those exceptions carry too much data and stick around).

If someone is raising a new Java exception, they might want to provide a cause. We can't easily do it automatically since we would have to modify the constructor call, and we probably wouldn't want to add something that magic. Unfortunately we also can't pass the exception, because the object in $! is a RubyException, not a subclass of Throwable.

I think we should open a new feature/bug to make it possible for a user to provide a cause to Java, but more and more I think doing it automatically is a mismatch.

I've confirmed that my tests pass on 9.1.13.0. I can't test for the memory leak until we get 9.1.13.0 into production, but I'll file another ticket (ideally with some kind of dev reproducible case) if we encounter that again.

Thanks for the fix!