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

ISPN-8893 Do not ignore the asking thread stack trace when reporting a remoting exception. #5815

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

yersan
Copy link
Contributor

@yersan yersan commented Mar 5, 2018

In general, the code relays on getting the remote exception when it is getting the result of completable future which manages the invoked command, so it is not possible to change the return type of the exception throwing directly an ExecutionException.

This is a tentative solution using the suppressed exception featured to encapsulate the current thread stack trace in the exception thrown.

Jira issue: https://issues.jboss.org/browse/ISPN-8893

…a remoting exception.

ISPN-8893 Modification additional classes
@ghost
Copy link

ghost commented Mar 5, 2018

Can one of the admins verify this patch?

@rvansa
Copy link
Member

rvansa commented Mar 6, 2018

LGTM. Have you checked if the logging infrastructure can handle cycle between cause and suppressed exceptions correctly? (While e.getCause() == e is common, e.getSuppressed()[n].getCause() == e could be troublesome)

@yersan
Copy link
Contributor Author

yersan commented Mar 6, 2018

Hi @rvansa , sorry, I think I don't understand well what do you mean with the logging infrastructure can handle cycle between cause and suppressed exceptions. Are you asking if all logging frameworks are able to print correctly a stack trace which has a suppressed Exception?, I mean, if log4j, slf4j, jboss-logging etc are able to print correctly the exception or not.
Could you give me more information about your concern?

As far as I understand, we won't need to deal with e.getSuppressed()[n].getCause() == e in infinispan code, that's the reason I don't understand well what's your point.

@rvansa
Copy link
Member

rvansa commented Mar 6, 2018

@yersan Yes, I meant log4j2 and jboss-logging (slf4j shouldn't print it itself). This is the first modification you've made:

Throwable cause = e.getCause();
cause.addSuppressed(e);

with this code cause.getSuppressed()[n] == e and since e.getCause() == cause ergo cause.getSuppressed()[n].getCause() == cause

@yersan
Copy link
Contributor Author

yersan commented Mar 6, 2018

@rvansa ok, thanks for the clarification, you are talking about the possible circular references printing the stack traces. I would say we are safe in that regard:

Log4j: Fixed since 2.4, infinispan is using log4j-2.8.2
https://issues.apache.org/jira/browse/LOG4J2-1046

jboss-logging: Fixed since 2.0.0, infinispan is using jboss-logging-3.3.1.Final:
https://issues.jboss.org/browse/LOGMGR-118

SLF4J, there is a problem fixed since 1.3.0-alpha5. It is not a direct dependency but it looks like infinispan is using SLF4J-1.7.22
https://jira.qos.ch/browse/LOGBACK-1027

Do you agree?

@rvansa
Copy link
Member

rvansa commented Mar 6, 2018

@yersan Thanks for the checks; apparently since these bugs exist being careful about that was substantiated.
And thanks for the contribution; integrating.

@rvansa rvansa merged commit c411422 into infinispan:master Mar 6, 2018
@yersan yersan deleted the bugs/ISPN-8893-master branch March 6, 2018 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants