Skip to content

Properly handle exceptions in threads created by MongoClient #1764

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

abalanonline
Copy link

@abalanonline abalanonline commented Jul 7, 2025

Hello dear mongo-java-driver maintainers,
Please allow me to contribute this bugfix. The problem with Throwable in this line is that
it catches errors (OutOfMemoryError in particular) and doesn't let JVM crash leaving it
in zombie state - unable to recover, unable to exit.

JAVA-5913

@abalanonline abalanonline requested a review from a team as a code owner July 7, 2025 19:54
@abalanonline abalanonline requested review from stIncMale and removed request for a team July 7, 2025 19:54
@rozza
Copy link
Member

rozza commented Jul 10, 2025

Thanks @abalanonline for the PR.

I've added JAVA-5913 to track this. It'll be triaged in due course.

Ross

@stIncMale stIncMale changed the title catching errors in DefaultServerMonitor Properly handle exceptions in threads created by MongoClient Jul 24, 2025
@stIncMale stIncMale self-assigned this Jul 24, 2025
@stIncMale stIncMale requested a review from rozza July 24, 2025 04:03
@stIncMale
Copy link
Member

stIncMale commented Jul 24, 2025

Hi @abalanonline, thank you for the PR.

  1. The place your identified is not the only place where we were incorrectly catching and swallowing Throwable.
  2. Also, there are a number of threads created by MongoClient that may be silently terminated due to a throwable. I updated those places such that we log a message and rethrow, allowing applications to handle such throwables via the default UncaughtExceptionHandler.

@abalanonline, @rozza please take a look, but hide whitespaces when reviewing to see the meaningful changes.

Copy link
Author

@abalanonline abalanonline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, catching the error, logging and rethrowing it will work. I can give verbal approval.
But since you asked for my opinion, I would kindly suggest removing all Error catching from the code and letting the JVM handle them. Alternatively, the user of the client library can handle such errors themselves if they are confident enough.

@stIncMale
Copy link
Member

suggest removing all Error catching from the code and letting the JVM handle them. Alternatively, the user of the client library can handle such errors themselves if they are confident enough.

The only behavioral difference between my approach and the approach you propose is logging. The issue with not logging is that when an internal MongoClient thread terminates due to an exception, the application that does not have the default UncaughtExceptionHandler configured, will have no easy way of knowing that the problem occurred. And the majority of application developers probably don't know about the internal MongoClient threads (why would they?), thus not necessarily suspecting that it may be useful to configure the default UncaughtExceptionHandler. This is why I deem log messages about unexpected termination of internal threads useful.

@abalanonline
Copy link
Author

I agree. Logging the error seems to be the best option.

@rozza rozza requested a review from Copilot July 24, 2025 15:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a critical bug where catching Throwable in background threads could prevent the JVM from properly handling fatal errors like OutOfMemoryError, leaving the application in a zombie state. The fix ensures fatal errors can propagate to terminate the JVM while still handling recoverable exceptions appropriately.

  • Replaces broad Throwable catches with more specific Exception catches in thread monitoring loops
  • Adds proper exception handling with logging and re-throwing for background worker threads
  • Improves error messages to provide more context about which component stopped working

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
MongoClient.java Adds exception handling to cursor cleaning thread with logging and re-throwing
TlsChannelStreamFactoryFactory.java Adds exception handling to TLS selector loop with logging and re-throwing
PowerOfTwoBufferPool.java Adds exception handling to buffer pool pruning thread with logging and re-throwing
LoadBalancedCluster.java Adds exception handling to wait queue handler thread with logging and re-throwing
DefaultServerMonitor.java Narrows exception handling from Throwable to Exception in monitoring loops, adds exception handling to main monitor thread
DefaultDnsSrvRecordMonitor.java Adds exception handling to DNS SRV record monitoring thread with logging and re-throwing
DefaultConnectionPool.java Updates error message to include object context
BaseCluster.java Adds exception handling to wait queue handler thread with logging and re-throwing
AsynchronousClusterEventListener.java Adds exception handling to event publishing thread with logging and re-throwing

rozza and others added 2 commits July 24, 2025 16:23
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…rverMonitor.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants