Added timeout for fetching windows keystores #445
Conversation
Hakky54
changed the title
|
| return keyStore; | ||
| }) | ||
| .thenApply(Optional::of) | ||
| .get(500, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
The timeout will make the CompletableFuture complete exceptionally with a TimeoutException, but the underlying thread will probably stay blocked.
After multiple calls, it may create a thread leak (supplyAsync uses the ForkJoin pool, which is not limited in the number of threads AFAIR).
If you want more control, you should not use CompletableFuture, but your own thread, and attempt to interrupt it after the timeout. But I am not even sure it would work, because the stacktrace showed that the process was blocked in native code (sun.security.mscapi.CKeyStore.loadKeysOrCertificateChains) and I have no idea if this could be interrupted.
There was a problem hiding this comment.
Yeah, you might be correct indeed. The underlying thread can still be active...
I will investigate whether the native code can be interrupted or else this PR will not resolve the issue at all.
Thank you for reviewing this PR.
There was a problem hiding this comment.
Correcting myself :
The timeout will make the CompletableFuture complete exceptionally with a TimeoutException
this is inaccurate. get(timeout) will throw a TimeoutException if the time is exceeded, but the CompletableFuture will still be not completed.
You could be tempted to do:
try {
future.get(timeout);
} catch (TimeoutException e) {
future.cancel(true);
}
The problem is that the "interruptIfRunning" flag is not used by the completable future framework. And the question about the actual effect of interrupting a native method call remains anyway.
Hakky54
deleted the
bug-fix/reduce-amount-of-time-to-load-windows-system-keystore-types
branch
No description provided.