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

AuthenticationFailure bad exception rethrowing #11022

Closed
pveentjer opened this issue Aug 2, 2017 · 2 comments
Closed

AuthenticationFailure bad exception rethrowing #11022

pveentjer opened this issue Aug 2, 2017 · 2 comments

Comments

@pveentjer
Copy link
Member

@pveentjer pveentjer commented Aug 2, 2017

class AuthenticationFuture {

    Connection get(int timeout) throws Throwable {
        if (!countDownLatch.await(timeout, TimeUnit.MILLISECONDS)) {
            throw new TimeoutException("Authentication response did not come back in " + timeout + " millis");
        }
        if (connection != null) {
            return connection;
        }
        assert throwable != null;
        throw throwable;<----
    }
}

You can't rethrow an exception from a different thread; it will lead to exceptions with very obscure stacktraces.

@sancar sancar added this to the 3.8.5 milestone Aug 2, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Aug 2, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Aug 2, 2017
@sancar sancar assigned sancar and unassigned sancar Aug 2, 2017
@sancar
Copy link
Member

@sancar sancar commented Aug 2, 2017

A quick look to solution seems to require synchronization on AuthenticatFuture.get method,
because using fixAsyncStackTrace(throwable, Thread.currentThread().getStackTrace()); with multiple calls to same AuthenticationFuture does not work.
I started to think that added value of the fix does not justify complexity that we are gonna add when fixing. @pveentjer What do you think ?

As a side node I realized we had a similar problem on invocationFuture, multiple call to InvocationFuture.get results in a exception with stacktrace repeated. If we are OK with that, solution is easy ;). Sample stacktrace to InvocationFuture.get called 3 times is as follows:

java.util.concurrent.ExecutionException: java.lang.Exception
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolveAndThrowIfException(InvocationFuture.java:90)
	at com.hazelcast.spi.impl.AbstractInvocationFuture.get(AbstractInvocationFuture.java:147)
	at com.hazelcast.spi.impl.operationservice.impl.Invocation_TaskDoneTest.when_invocation_finishWithException_getTwice(Invocation_TaskDoneTest.java:95)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:105)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:97)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.Exception
	at com.hazelcast.spi.impl.operationservice.impl.Invocation_TaskDoneTest.when_invocation_finishWithException_getTwice(Invocation_TaskDoneTest.java:78)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:105)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:97)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:745)
	at ------ submitted from ------.(Unknown Source)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolve(InvocationFuture.java:120)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolveAndThrowIfException(InvocationFuture.java:77)
	at com.hazelcast.spi.impl.AbstractInvocationFuture.get(AbstractInvocationFuture.java:147)
	at com.hazelcast.spi.impl.operationservice.impl.Invocation_TaskDoneTest.when_invocation_finishWithException_getTwice(Invocation_TaskDoneTest.java:81)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:105)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:97)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:745)
	at ------ submitted from ------.(Unknown Source)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolve(InvocationFuture.java:120)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolveAndThrowIfException(InvocationFuture.java:77)
	at com.hazelcast.spi.impl.AbstractInvocationFuture.get(AbstractInvocationFuture.java:147)
	at com.hazelcast.spi.impl.operationservice.impl.Invocation_TaskDoneTest.when_invocation_finishWithException_getTwice(Invocation_TaskDoneTest.java:88)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:105)
	at com.hazelcast.test.FailOnTimeoutStatement$CallableStatement.call(FailOnTimeoutStatement.java:97)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:745)
	at ------ submitted from ------.(Unknown Source)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolve(InvocationFuture.java:120)
	at com.hazelcast.spi.impl.operationservice.impl.InvocationFuture.resolveAndThrowIfException(InvocationFuture.java:77)
	... 14 more
@pveentjer
Copy link
Member Author

@pveentjer pveentjer commented Aug 3, 2017

I would stay away from stacktrace rewriting. We should get rid of it in the future once we can fix the exception throwing behavior of our invocation future (breaking compatibility).

Either wrap an ExecutionException around it and let the caller peel the inner exception if he cares for it. I think this is the most sane approach.

Or wrap it in an AuthenticationException; since the AuthenticationFuture isn't implementing Future interface, you can wrap any exception around it you see fit.

sancar added a commit to sancar/hazelcast that referenced this issue Aug 3, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Aug 3, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Aug 3, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Aug 3, 2017
@sancar sancar closed this in #11027 Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.