BUG: Thread-Unsafe Static HashMap in HikariDbSource - Race Condition in Connection Pool Management

[minguyen9988]

Summary

HikariDbSource.java uses static HashMap for both instance (connection pool map) and connectionPool (connection map). The getInstance() method at lines 108-115 uses a check-then-act pattern (containsKey → put) that is not atomic, creating a race condition when multiple threads request connections to new databases simultaneously.

Location

• File: sink-connector/src/main/java/com/altinity/clickhouse/sink/connector/db/HikariDbSource.java
• Lines: 32, 37, 108-115

Current Code

// Line 32: Static mutable HashMap - not thread-safe
private static Map<String, HikariDataSource> instance = new HashMap<>();

// Line 37: Static mutable HashMap - not thread-safe
private static Map<String, Connection> connectionPool = new HashMap<>();

// Lines 108-115: Check-then-act race condition
public static HikariDataSource getInstance(
        SinkConnectorDataSource dataSource, String databaseName,
        ClickHouseSinkConnectorConfig config) {
    // ...
    if (instance.containsKey(databaseName)) {      // CHECK
        return instance.get(databaseName);          // may return null if removed between check and get
    } else {
        HikariDataSource hikariDataSource = createConnectionPool(
                dataSource, databaseName, config);  // ACT
        instance.put(databaseName, hikariDataSource);
    }
    return instance.get(databaseName);
}

Problem

1. HashMap is not thread-safe. Concurrent modifications from multiple threads can cause infinite loops in the internal hash table, ConcurrentModificationException, or silent data corruption.
2. The getInstance() method has a TOCTOU (Time-of-Check-to-Time-of-Use) race: containsKey() and put() are not atomic. Two threads requesting a pool for the same new database could both create pools, causing a resource leak.
3. The closeDatabaseConnection() method at line 211-219 also has a check-then-act race.
4. close() at line 192 iterates instance.values() without synchronization, risking ConcurrentModificationException during shutdown.

Impact

• Severity: High (P1)
• Effect: Connection pool leak (duplicate pools created), potential ConcurrentModificationException, potential infinite loop in HashMap
• Scope: Any multi-threaded connector deployment with multiple databases

Suggested Fix

1. Replace HashMap with ConcurrentHashMap for both instance and connectionPool
2. Use computeIfAbsent() in getInstance() for atomic check-and-create:

private static Map<String, HikariDataSource> instance = new ConcurrentHashMap<>();

public static HikariDataSource getInstance(...) {
    return instance.computeIfAbsent(databaseName, 
        db -> createConnectionPool(dataSource, db, config));
}

3. Synchronize or use atomic operations in close() and closeDatabaseConnection()

References

• doc/issues/CONCURRENCY-BUGS.md (BUG-CONC-2, related to unsynchronized cache access)
• doc/issues/ANTI-PATTERNS.md (AP-HIGH-2: Static State, AP-MED-4: Mutable Static State)