-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Synchronizing System.getProperties in generatePoolName / Number creates a deadlock-able window. #2104
Comments
I can think of potential sequence of events that could lead to a deadlock:
There is a possibility of both Thread A and Thread B are in a deadlock situation. |
That is exactly what's happening. There are several thousand third party classes that are being processed + initialized in another thread and some of them call System.getProperties. so even though we have a small window, in practice, in a slower, containerized environment with limited cores, we're seeing the deadlock occur approximately 1/4 times. |
If someone creates a pull request with a fix for this issue, I will gladly merge it. |
We can fix this in a variety of ways, but I want to be clear on the potential side effects for other users of the library other than my use case. There is always one single HikariCP per JVM, correct? two wars with different database configurations still have a single HikariCP ? If that's right, I'm happy to deliver a PR. |
@kialambroca There can be multiple HikariCP libraries loaded into a single JVM. Two wars is a perfect example of the use-case that the current code is attempting to account for. |
// AS-IS
// Pool number is global to the VM to avoid overlapping pool numbers in classloader scoped environments
threadPoolName = "HikariPool-1, 2, 3, ...";
// TO-BE
static UUID = system.getUUID();
AtomicInteger poolNum;
public generatePoolName() {
threadPoolName = "HikariPool-{UUID}-{poolNum.incrementAndGet()}"; // HikariPool-{UUID}-1, 2, 3, ...
// On same JVM, war1 pool name: HikariPool-{war1's UUID}-1, 2, 3, ..
// war2 pool name: HikariPool-{war2's UUID}-1, 2, 3, ..
} Hi @lfbayer ! I think you may already consider this way, but can't we just change threadPoolName format like above? With WDYT? :) |
Hello, @lfbayer I've been investigating the implementation of a lock mechanism using system properties and found it quite challenging. In search of an alternative, I developed a port-based lock approach to address race conditions from classes loaded multiple times by different classloaders. I've documented this approach in an example and test case here: PortLock Example. While it's still a work in progress, I'm keen to hear your thoughts on this direction or if we should consider refining the poolName format as an alternative. Looking forward to your feedback. Thank you. |
Since Hikari is a library, it should not rely on synchronizing on any objects not within it's own scope, Interactions between code either more shallow or deeper in the stack can create opportunities for a deadlock. It certainly shouldn't synchronize on on the System.getProperties() Properties object.
The code appears to facilitate sharing of the pool name across classloaders. This could be obviated by using the hashcode of a specific bootloader loaded class.
HikariCP/src/main/java/com/zaxxer/hikari/HikariConfig.java
Line 1169 in 2e07be7
The text was updated successfully, but these errors were encountered: