-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Log non-serialisable exception before it is discarded #26323
base: master
Are you sure you want to change the base?
Log non-serialisable exception before it is discarded #26323
Conversation
…re it is discarded.
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Internal PR hazelcast/hazelcast-mono#1780 |
if (rootObject instanceof Throwable throwable) { | ||
LOGGER.warning("Failed to serialize '" + clazz + "'. It will be logged here and " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
if (rootObject instanceof Throwable throwable) { | |
LOGGER.warning("Failed to serialize '" + clazz + "'. It will be logged here and " | |
if (rootObject instanceof Throwable throwable) { | |
LOGGER.warning("Failed to serialize throwable '" + clazz + "'. It will be logged here and " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this change makes sense, i am curious to hear others if any other solution is possible to make an exception easy to track that's failed to be serialized
I think there is no other way; we can't add them to supressed exceptions, because those will be also serialized (and we already know, that we can't serialize it). Logging makes most sense. |
run-lab-run |
The job Click to expand the log file-------------------------- ---------SUMMARY---------- -------------------------- [ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.4.1:enforce (enforce-tools) on project hazelcast-root: -------------------------- ---------ERRORS----------- -------------------------- [ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.4.1:enforce (enforce-tools) on project hazelcast-root: -------------------------- [ERROR] Rule 0: org.apache.maven.enforcer.rules.version.RequireJavaVersion failed with message: -------------------------- [ERROR] Detected JDK version 11.0.18 (JAVA_HOME=/usr/share/jdks/oracle-11) is not in the allowed range [17,). -------------------------- [ERROR] -> [Help 1] -------------------------- [ERROR] -------------------------- [ERROR] Re-run Maven using the -X switch to enable full debug logging. -------------------------- [ERROR] -------------------------- [ERROR] For more information about the errors and possible solutions, please read the following articles: -------------------------- [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException -------------------------- [ERROR] -------------------------- [ERROR] After correcting the problems, you can resume the build with the command -------------------------- [ERROR] mvn -rf :hazelcast-root -------------------------- |
If the real issue is that the cause of the original exception is lost, perhaps it'd be better to add the exceptions' message to the new Currently, something else may catch the |
Can you give an example? The new log will only run when there is a Throwable that could not be serialized. Also I think only message of the exception is not enough, because the author wants to track the exception. So I think stacktrace is important for that.
As much as I don't like a lot of new configuration, we can put this behind a property that would be disabled by default. |
Two (obscure) cases come to mind:
I'm sure there's a textual representation we could agree on that would have that information - effectively here were saying - if you can't serialize the exception over-the-wire, serialize to a file instead (in the form of a log message). |
What if we log this to a dedicated logger (e.g. |
Note that these will log only if the object is a Throwable, which is not very likely imo.
Can be possible but I think Jack does not like the default behaviour of logging those throwables that can't be serialized. Adding dedicated logger will still log them by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: The problem is about at the client side not seeing exception details, right? We actually put the stack trace information into the error codec, and it was deserialized and set at the client exception factory while exception is created. Was this not enough?
If you also want to communicate the details of the object that can not be serialized and if it is of type Throwable
, the we can set this object as the cause for the constructed HazelcastSerializationException
and if there is any other exception we throw during the serialization. At the end, user will see top level HazelcastSerializationException`, a cause and the cause of the cause which is the throwable object. Then, you do not need to do logging, does this make sense?
nope not enough. We don't put any other info than the class name about the object to be serialized's (if it's Throwable) in to HazelcastSerializationException's message.
this seems ok to me, however what if it already has a cause? Also it's not the cause of serialization exception actually, but the object tried to be serialized. We need to make this clear |
The exception is created during serialization by us, hence we control the cause part of the exception and we can check the places we throw during serialization and arrange the cause properly to include this object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Staling my review until we understand the use case. We believe Throwables should never be returned from our APIs as responses.
So @tommyk-gears, can you please share your executor code reproducer that tries to serialize a Throwable and fails to do so? We think you don't need this change in this PR, but can simply do one of the following:
- If there is an error, just throw it in the executor service's task, instead of returning it as a response. It'll skip serialization and be returned to the user via ErrorCodecs
- If you really want to log it in the member side, you can add try/catch in the executable task in ExecutorService and log it in the catch() block.
Discussed with @srknzl and we concluded that why we need to try serialize a throwable in the first place? Normally, in a callable, you either return a real non-throwable object or you throw an exception. Hence, why is the throwable used as the return value of a callable? |
There might be some mis-understanding here. I am not using hazelcast-client, but executing stuff on a Here is a quick "reproducer" (pastable into static class NonSerializableObject {
}
static class NonSerializableException extends RuntimeException {
private final Object o = new NonSerializableObject();
NonSerializableException(String message) {
super(message);
}
}
static class NonSerializableExceptionThrowingCallable implements Callable<String> {
@Override
public String call() {
throw new NonSerializableException("I want to see this exception including its stacktrace etc in the log");
}
}
@Test
public void test_executeRunnable_onLiteMember_throwsNonSerializableException() {
final TestHazelcastInstanceFactory factory = createHazelcastInstanceFactory(2);
final HazelcastInstance lite1 = factory.newHazelcastInstance(liteConfig);
final HazelcastInstance other = factory.newHazelcastInstance(smallInstanceConfig());
final String name = randomString();
final IExecutorService executor = other.getExecutorService(name);
Future<String> future = executor.submitToMember(new NonSerializableExceptionThrowingCallable(),
lite1.getCluster().getLocalMember());
ExecutionException executionException = assertThrows(ExecutionException.class, future::get);
Logger.getLogger(ExecutorServiceLiteMemberTest.class).severe(executionException);
} The relevant log parts when running the test are; First the logging I added in this PR;
And the then logging of the exception in the last line of the test - just to show what the received exception looks like on the "submitter side";
|
@tommyk-gears Yeah we thought you used hz client. Will look into your code, thanks for sharing |
This is the log when there is a Hz client used instead of "other" member:
Is this good enough for you btw? |
If serialisation of an exception fails, then produce a log message including the exception, before it is discarded. This will help developers find the source of the non-serialisable exception.
Fixes #26322
Breaking changes (list specific methods/types/messages): No breaking changes
Checklist:
Team:
,Type:
,Source:
,Module:
) and Milestone setAdd to Release Notes
label if changes should be mentioned in release notes orNot Release Notes content
if changes are not relevant for release notes@Nonnull/@Nullable
annotations@since
tags in Javadoc