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

Replaced the synchronized use of System.getProperties with AtomicInteger in pool name generation #2115

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

echo724
Copy link

@echo724 echo724 commented Sep 29, 2023

Fix: #2104

The objective of using synchronized was to avoid overlapping in the generation of HikariPool numbers, likely in sequential operations following a call to System.getProperties. However, synchronizing these operations could potentially lead to deadlocks. Therefore, I considered an alternative that does not rely on synchronized access to the System properties. Using AtomicInteger for Pool number generation eliminates the risk of overlap without the deadlock risk associated with synchronized blocks.

  1. Reason for using AtomicInteger: 
    AtomicInteger serves as a JVM level shared value to handle concurrency safely while generating unique numbers within a single JVM. This ensures that each HikariConfig instance is assigned a unique pool name, even in situations where multiple threads attempt to generate pool names simultaneously.

  2. Reason for using AtomicInteger over UUID:
    While UUID does provide uniqueness, it comes at a higher generation cost and complexity. Conversely, using AtomicInteger allows for relatively lightweight and swift generation of unique numbers in asynchronous and concurrent environments. Furthermore, using AtomicInteger alleviates the need to handle synchronization during pool name generation, thus reducing code complexity while addressing concurrent access issues.

The generation code is as follows:

// Added priavte static field POOL_NUMBER to track pool number across the instances
private static final AtomicInteger POOL_NUMBER = new AtomicInteger(0);
...
private String generatePoolName() {
    final var prefix = "HikariPool-";
    // The pool number will be incremented atomically
    final var next = String.valueOf(poolNumber.incrementAndGet());
    return prefix + next;
}
...

@echo724 echo724 changed the title Replaced synchronized with AtomicInteger in pool name generation Replaced synchronized with AtomicInteger in pool name generation Sep 29, 2023
@echo724 echo724 changed the title Replaced synchronized with AtomicInteger in pool name generation Replaced the synchronized use of System.getProperties with AtomicInteger in pool name generation Sep 29, 2023
Copy link
Collaborator

@lfbayer lfbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is insufficient as it doesn't handle the case where there are multiple classloaders loading their own hikaricp classes. In that case you would have multiple instances of POOL_NUMBER (one per classloader).

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.

Synchronizing System.getProperties in generatePoolName / Number creates a deadlock-able window.
2 participants