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

Fix argon and balloon blocking application shutdown #151

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

phinner
Copy link
Contributor

@phinner phinner commented Apr 17, 2024

If you set the parallelism of Argon2Function or BalloonHashingFunction above 1 and that you hash something, a non daemon ExecutorService will be created, which can block the application shutdown if there is no explicit System.exit.

This PR fixes this by making the threads of the created ExecutorService daemon.
And also add names and a dedicated thread group for password4j threads, making debugging that kind of issue MUCH easier (I had to trace every thread pool creation to figure the issue out).

@firaja
Copy link
Member

firaja commented Apr 18, 2024

Hi @phinner thank you for your awesome work!
Also having a dedicated thread group is a good idea.
Just to have more context on your application, in what case this issue happened to you?

@firaja firaja added this to the 1.8.2 milestone Apr 18, 2024
@phinner
Copy link
Contributor Author

phinner commented Apr 18, 2024

Just to have more context on your application, in what case this issue happened to you?

Spotted it in my plugin for the game Mindustry after updating all my dependencies (including password4j moving from 1.7 to 1.8).
As you can see here, no explicit exit after the game loop ends.
And given the fact that I use a parallelisms higher than 1 for my account system, the server blocks.

Now, after a good night, I had more time to think about the use of ExecutorService in the first place.
What if a user creates several slightly different argon2 functions, a lot of threads would be created too, unused most of the time. So what if there was a shared thread pool for all functions instead, with additional config, such as:

int minThreads = PropertyReader.readInt("global.threads.min", AVAILABLE_PROCESSORS, "The minimum number of threads is not defined");
if (minThreads < 0) {
    throw new IllegalArgumentException("The minimum number of threads is negative");
}
int maxThreads = PropertyReader.readInt("global.threads.max", AVAILABLE_PROCESSORS, "The maximum number of threads is not defined");
if (maxThreads < 0) {
    throw new IllegalArgumentException("The maximum number of threads is negative");
}
if (minThreads > maxThreads) {
    throw new IllegalArgumentException("The minimum number of threads is above the maximum");
}

THREAD_POOL = new ThreadPoolExecutor(
    minThreads,
    maxThreads,
    60L,
    TimeUnit.SECONDS,
    new LinkedBlockingQueue<>(),
    runnable -> {
        Thread thread = new Thread(THREAD_GROUP, runnable, "password4j-worker-" + THREAD_COUNTER.getAndIncrement());
        thread.setDaemon(true);
        return thread;
    }
);

Since it's essentially a new feature, this would be in 1.9, thus me asking first.

@firaja
Copy link
Member

firaja commented Apr 19, 2024

I see your point, very interesting. Take in account that 95% of the time systems will use 1 algorithm with 1 configuration.
I'm thinking about web applications and authentication systems in general.
Do you see, in your context, the need to have multiple algorithms with different configuration running in a single instance?

@phinner
Copy link
Contributor Author

phinner commented Apr 19, 2024

Do you see, in your context, the need to have multiple algorithms with different configuration running in a single instance?

In my case, I use one argon function for passwords and another for session tokens.
And I use a PBKDF2 function since it was the previous password hash function.

@firaja firaja merged commit c21a064 into Password4j:master Apr 23, 2024
5 of 6 checks passed
@phinner phinner deleted the fix/blocking-threads branch April 23, 2024 13:41
@firaja
Copy link
Member

firaja commented May 1, 2024

Hi @phinner this is now part of 1.8.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants