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

KPL Silently Swallows Exceptions from Deamon Threads #111

Closed
jeremysears opened this issue May 26, 2017 · 5 comments · Fixed by #112
Closed

KPL Silently Swallows Exceptions from Deamon Threads #111

jeremysears opened this issue May 26, 2017 · 5 comments · Fixed by #112
Labels

Comments

@jeremysears
Copy link
Contributor

The Deamon class will swallow and not report any exceptions that are throw by its deamon threads. The deamon threads are initiated by running ExecutorService.submit(Runnable). The submit method returns a Future object. If the Runnable throws, its exception is added to the Future object. However, Deamon never looks at the Future objects, so any errors are never reported. Instead of calling ExecutorService.submit(Runnable), as currently written, this should probably call ExecutorService.execute(Runnable) instead. Changing this to use execute allows the Thread's UncaughtExceptionHandler or the global default UncaughtExceptionHandler to properly handle the failure. At a minimum this allows the error to be logged.

This situation could arise in several scenarios. For example, if a deamon thread runs into an OutOfMemoryError or if a RuntimeException thrown from a custom AWSCredentialsProvider used by the credential refresh thread.

In the event that one of these threads throws, the KPL will no longer operate correctly. Consequently, some restart logic, or a hard shutdown should probably be initiated in failure conditions.

@jeremysears
Copy link
Contributor Author

You can reproduce this by hardcoding an exception in any of the Deamon's thread's run methods. The thread will abort, but nothing will be logged.

@jeremysears
Copy link
Contributor Author

I've prepared a pull request which will at least allow any errors to be logged. I'll submit it once it passes our staging environment. However, I haven't addressed restart/shutdown logic in the pull request.

@pfifer
Copy link
Contributor

pfifer commented Jun 6, 2017

Thanks for reporting this. I'm looking at your PR as well.

@jeremysears
Copy link
Contributor Author

What is the ETA for merging the related PR?

@lipmice
Copy link

lipmice commented Aug 23, 2017

I'm running into this as well. It would be good to have these errors logged to flag the error and enable cleanup/restart.

pfifer added a commit to pfifer/amazon-kinesis-producer that referenced this issue Nov 14, 2017
* Added Windows support
  The 0.12.x version now supports Windows.
  The Windows version is currently mastered on the branch `windows`, which will be merged at a later date.
  The build instructions for Windows are currently out of date, and will be updated at a later date.__
  * Issue awslabs#113
  * Issue awslabs#74
  * Issue awslabs#73
* Removed the libc wrapper
  The libc wrapper lowered the required version of glibc.  The KPL is now built with an older version of libc, which removes the need for the wrapper.
  * PR awslabs#139
* Set the minimum required version of macOS to 10.9.
  The KPL is now built against macOS 10.9.
  * Issue awslabs#117
  * PR awslabs#138

* Allow exceptions to bubble to the thread exception handler for Daemon threads.
  Exceptions that occur on daemon threads will now be allowed to propagate to the thread exception handler.  This doesn't provide any additional monitoring or handling of thread death.
  * PR awslabs#112
  * Issue awslabs#111
* Updated `amazon-kinesis-producer-sample` to use the correct properties in its configuration file.
  * PR awslabs#120
  * Issue awslabs#119
* Updated documentation of `AggregationMaxSize` to match actual Kinesis limits.
  * PR awslabs#133
* Added support for setting `ThreadingModel`, and `ThreadPoolSize` using a properties file.
  * PR awslabs#134
  * Issue awslabs#124
* Extracted `IKinesisProducer` from `KinesisProducer` to allow for easier testing.
  * PR awslabs#136
pfifer added a commit that referenced this issue Nov 14, 2017
* Added Windows support
  The 0.12.x version now supports Windows.
  The Windows version is currently mastered on the branch `windows`, which will be merged at a later date.
  The build instructions for Windows are currently out of date, and will be updated at a later date.__
  * Issue #113
  * Issue #74
  * Issue #73
* Removed the libc wrapper
  The libc wrapper lowered the required version of glibc.  The KPL is now built with an older version of libc, which removes the need for the wrapper.
  * PR #139
* Set the minimum required version of macOS to 10.9.
  The KPL is now built against macOS 10.9.
  * Issue #117
  * PR #138

* Allow exceptions to bubble to the thread exception handler for Daemon threads.
  Exceptions that occur on daemon threads will now be allowed to propagate to the thread exception handler.  This doesn't provide any additional monitoring or handling of thread death.
  * PR #112
  * Issue #111
* Updated `amazon-kinesis-producer-sample` to use the correct properties in its configuration file.
  * PR #120
  * Issue #119
* Updated documentation of `AggregationMaxSize` to match actual Kinesis limits.
  * PR #133
* Added support for setting `ThreadingModel`, and `ThreadPoolSize` using a properties file.
  * PR #134
  * Issue #124
* Extracted `IKinesisProducer` from `KinesisProducer` to allow for easier testing.
  * PR #136
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 a pull request may close this issue.

3 participants