-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
refactor: reusable thread pool to decrypt keystores #5623
Conversation
// Since it is perfectly fine to have listeners > 10 | ||
setMaxListeners(Infinity, abortController.signal); | ||
|
||
onGracefulShutdownCbs.push(async () => abortController.abort()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abort has to be pushed to graceful shutdown callbacks before invoking getSignersFromArgs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting DEBUG=threads:pool:*
is pretty helpful to analyse thread and task lifecycle
{ | ||
// Adjust worker pool size based on keystore count | ||
size: Math.min(keystoreCount, maxPoolSize), | ||
// Decrypt keystores in sequence, increasing concurrency does not improve performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did several test runs with different concurrency but it seems to just overload the threads and throttle the CPU
@@ -37,9 +37,6 @@ export async function readPassphraseOrPrompt(args: {importKeystoresPassword?: st | |||
}, | |||
]); | |||
|
|||
// eslint-disable-next-line no-console | |||
console.log("Password is correct"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This console.log is incorrect and not helpful as password verification is done later in the process
Performance Report✔️ no performance regression detected Full benchmark results
|
d608c94
to
0ca782a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the error handling.
Also having a cancelable decryptKeystoreDefinitions
is really nice.
Yeah, that one is quite important because if you force close the process then the .lock files are not removed and need to be manually cleaned up. |
0ca782a
to
6eb8210
Compare
6eb8210
to
a529ddf
Compare
🎉 This PR is included in v1.9.0 🎉 |
Motivation
Decrypting keystores is required in multiple places
This PR makes the thread pool implementation reusable, improves pool lifecycle management and error handling.
Description
Reusable thread pool to decrypt keystores, addresses the following issues
Current error handling
Current error thrown if decryption of keystores fails is not that helpful if there are multiple. There is no indication on which keystore file is affected and only reports one error.
Updated error handling
Updated error handling provides more details on which keystore import failed and why it failed.
Single keystore import error
Multiple keystore import errors
Single error, multiple aborted due to error
Multiple errors, multiple aborted due to errors