Skip to content

Fix #163 thread leak #165

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

Merged
merged 2 commits into from
Jun 30, 2025

Conversation

halissontorres
Copy link
Contributor

Fix Tomcat thread leak warning by using daemon threads

Problem

Password4j creates non-daemon worker threads that cause memory leak warnings in web containers like Tomcat:

WARNING [main] org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads 
The web application [Password4jThreadIssue_war_exploded] appears to have started a thread 
named [password4j-worker-19] but has failed to stop it. This is very likely to create a memory leak.

Password4jThreadIssue

This occurs because non-daemon threads prevent the web application from unloading cleanly during redeployment or shutdown.

Root Cause

In Utils.createExecutorService(), the thread factory explicitly sets thread.setDaemon(false), creating non-daemon threads that:

  • Prevent JVM shutdown until explicitly terminated
  • Cause web containers to report memory leak warnings
  • Can prevent web application classloaders from being garbage collected

Solution

Changed thread.setDaemon(false) to thread.setDaemon(true) in the thread factory within Utils.createExecutorService().

Daemon threads:

  • Automatically terminate when the JVM shuts down
  • Don't prevent web application unloading
  • Don't cause container memory leak warnings
  • Don't interfere with Password4j's existing shutdown hook

Changes Made

  1. Utils.java: Changed setDaemon(false) to setDaemon(true) in createExecutorService()
  2. IssuesTest.java: Added issue163() test method to verify the fix

Testing

The new test issue163() verifies that:

  • Password4j worker threads are created when performing hash operations
  • All password4j-worker-* threads are daemon threads
  • No memory leak warnings should occur in web containers

Test Results

Before fix: Test fails - threads are non-daemon
After fix: Test passes - threads are daemon

Backward Compatibility

This change is fully backward compatible:

  • No API changes
  • No behavioural changes for end users
  • Password4j functionality remains identical
  • Existing shutdown hook continues to work

Impact

  • Fixes Tomcat thread leak warnings
  • Enables clean web application deployment/undeployment
  • Allows proper JVM shutdown
  • No performance impact
  • Maintains all existing functionality

Testing Instructions

  1. Run the new test: mvn test -Dtest=IssuesTest#issue163
  2. Deploy to Tomcat and verify no thread leak warnings appear
  3. Existing functionality tests should continue to pass

Resolves issue #163

@firaja firaja merged commit 4c0e433 into Password4j:master Jun 30, 2025
5 of 6 checks passed
@firaja firaja added this to the 1.8.4 milestone Jul 1, 2025
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