Conversation
| --stacktrace --info --continue ${TESTS} | tee -a logs.txt | ||
|
|
||
| if grep -q 'LEAK:' logs.txt ; then | ||
| echo "Netty Leak detected, please inspect build log" |
There was a problem hiding this comment.
sorry for being lazy, but I always miss this error message in evergreen logs, all it does it makes it red, we can also change it and redirect to error output, it can be moved into a separate PR
There was a problem hiding this comment.
I made a change in ByteBuf prs to make it standout more. - Lets remove from this PR but I like the idea of making it red as well.
|
Could you add some detail to the description of the git commits. Reviewing the git log wouldn't provide any real information regarding this change or its motivations. |
|
|
||
| @Override | ||
| public void execute(final Runnable r) { | ||
| executor.execute(()->{ |
There was a problem hiding this comment.
| executor.execute(()->{ | |
| executor.execute(() -> { |
|
|
||
| for (int i = 0; i < 50; i++) { | ||
| gridFSService.submit(exportFile(latch, i)); | ||
| gridFSService.execute(exportFile(latch, i)); |
There was a problem hiding this comment.
Are we relying on the JVM own built‑in default handler to handle callback exceptions?
There was a problem hiding this comment.
We replace ExecutorService.submit with Executor.execute because only the latter allows an exception that originates from the executed task to be propagated as an uncaught exception (submit catches it and places in the returned Future, which we do not inspect). Once there is an uncaught exception, ThreadGroup.uncaughtException is called, which emits the information about the uncaught exception to the standard error stream.
The above happens only if the thread does not have an explicit, UncaughtExceptionHandler, and there is no default uncaught exception handler.
Thus with this change, uncaught exceptions won't stay unnoticed anymore:
- at the very least the info about them will be emitted to the standard error stream;
- an application may register the default uncaught exception handler to change that behavior.
- And it absolutely should. The majority of applications should terminate when a
VirtualMachineErrorhappens, because such errors, being unexpected and asynchronous on top of that, may cause application invariants to be violated.
- And it absolutely should. The majority of applications should terminate when a
There was a problem hiding this comment.
Our test code is "the application". So ideally, we should register a default uncaught exception handler that logs uncaught exceptions using our logging, and terminates the VM if it's a VirtualMachineError (it should also try and log such errors, but make sure the VM terminates even if logging fails).
I'll create a technical debt ticket for this.
There was a problem hiding this comment.
|
@strogiyotec I updated the PR description so that it points to the ticket, and also to the relevant comment in the previous PR. |
There was a problem hiding this comment.
Let's also look at all the places where we call methods of ScheduledExecutorService, and do not handle the exceptions from the returned Futures:
- If easily doable, we should handle the exceptions from those
Futures. But I suspect, if most if not all cases it is not really doable. - If not, we should make sure that the
Runnable/Callables we schedule catch everything, logExceptions they can't really handle, and handle allErrors by callingThread.currentThread().getThreadGroup().uncaughtException(e, Thread.currentThread())
Furthermore, everywhere we catch Throwable (we don't explicitly catch Errors or any subtypes, I checked) in production code, we should make sure that if it's an Error:
- It is either re-thrown / put into a
Future/ fed into a callback (if that's what we do) as is, without being wrapped into anything. - Or, if re-throwing is not an supposed to happen, is handled via
Thread.currentThread().getThreadGroup().uncaughtException(e, Thread.currentThread()).
The two above suggestions ensure that the default uncaught exception handler configured by an application will have a chance to handle all Errors that originate from our code. And if there no uncaught exception handler is configured, the Errors at least will be handled the same way the Java SE handles all uncaught exceptions by default.
| TimeUnit.MILLISECONDS, | ||
| c -> group.submit(() -> handler.completed((int) c, attach)), | ||
| e -> group.submit(() -> handler.failed(e, attach))); | ||
| c -> group.execute(() -> handler.completed((int) c, attach)), |
There was a problem hiding this comment.
This code comes from TLS Channel, which we "vendor" as part of the driver, though our vendored code deviates from the original.
@strogiyotec, let's create a TLS Channel issue suggesting this change? Of course, we'll need to explain the reasoning behind it. Or, if you feel especially generous, you could even open a PR for that repo :)
|
I updated my comment #1893 (review). Since there are no notifications about updates, I am leaving another comment as a way to notify. |
4 participants
The relevant comment in a different PR that lead to creating this one: #1885 (comment) (the original idea was to do the changes in this PR as part of #1885).
JAVA-5907