Multithreading and Code Quality Review

Status: COMPLETE

Completed: 2026-02-19

All 24 categories of multithreading and code quality findings have been addressed across 54 files (+1354/-1007 lines).

---

Executive Summary

A comprehensive review of the TQPro codebase identified and resolved multithreading safety issues, resource leaks, and code quality problems across all modules. The changes ensure thread-safe operation in both single-instance and multi-instance (clustered) deployments.

---

1. Hibernate Session Leak Fixes

Problem

Over 50 methods across 12 files opened Hibernate sessions without guaranteed cleanup. If an exception occurred between openSession() and session.close(), the session (and its database connection) would leak.

Solution

Wrapped all session usage in try-finally blocks to guarantee cleanup:

// Before (unsafe)
Session session = NTSDBSession.openSession();
List<?> results = session.createQuery("...").list();
session.close();

// After (safe)
Session session = NTSDBSession.openSession();
try {
    List<?> results = session.createQuery("...").list();
    return results;
} finally {
    session.close();
}

Affected Files

Module	File	Methods Fixed
tqapp	NTSCruiseService.java	~15 methods
tqapp	NTSGroupService.java	~10 methods
tqapp	NTSEntityReadService.java	2 methods
tqapp	NTSEntityWriteService.java	3 methods
tqapp	NTSHotelService.java	~5 methods
tqapp	NTSMarketingService.java	~3 methods
tqapp	NTSHotelFacade.java	~3 methods
tqapp	CatalogFacade.java	~3 methods
tqamds	AmdDBSession.java	Session factory management
tqryb2b	RaynaDBSession.java	Session factory management
tqtiqets	TiqetsDBSession.java	Session factory management
tqgglbl	GoGlobalDBSession.java	Session factory management

---

2. Transaction Rollback Safety

Problem

Database transactions were committed but not rolled back on exception. If an error occurred after beginTransaction() but before commit(), the transaction would remain open, potentially blocking other operations.

Solution

Added transaction.rollback() in catch/finally blocks:

Transaction tx = session.beginTransaction();
try {
    // ... operations ...
    tx.commit();
} catch (Exception e) {
    if (tx != null && tx.isActive()) {
        tx.rollback();
    }
    throw e;
}

Affected Files

File	Methods Fixed
NTSEntityWriteService.java	create, update, delete
NTSCruiseService.java	~5 write methods
NTSGroupService.java	~4 write methods
NTSHotelService.java	~2 write methods
NTSMarketingService.java	~1 write method
CatalogFacade.java	catalog update methods

---

3. Session Sharing to Avoid Multiple Opens

Problem

Some methods opened multiple Hibernate sessions for related operations within the same logical unit of work, wasting connections and risking inconsistency.

Solution

Refactored to pass a shared session to internal methods:

File	Change
NTSCruiseService.java	Sub-queries share caller's session
NTSGroupService.java	Related entity loads share session
CatalogFacade.java	Batch operations share session

---

4. Concurrent Data Structure Upgrades

Problem

Several HashMap instances were accessed from multiple threads without synchronization, risking data corruption (infinite loops in JDK 7, data loss in JDK 8+).

Solution

Replaced with ConcurrentHashMap:

File	Field	Before	After
StaticMapCache.java	_caches	HashMap	ConcurrentHashMap
CartHolder.java	carts	HashMap	ConcurrentHashMap
OdooServiceFactory.java	service map	HashMap	ConcurrentHashMap
PropertyList.java	properties	HashMap	ConcurrentHashMap

---

5. Volatile Singleton Fields

Problem

Singleton fields using double-checked locking were missing the volatile keyword. Without volatile, the JVM may reorder writes such that one thread sees a partially constructed object.

Solution

Added volatile to all singleton instance fields:

File	Field
NTSServiceFactory.java	instance
CartHolder.java	instance
TlinqEntityFactory.java	instance
AppConfig.java	instance
ClientConfig.java	instance
OdooClientConfig.java	instance
JSON.java	gson

---

6. Thread-Safe DateFormat

Problem

SimpleDateFormat instances were shared across threads. SimpleDateFormat is NOT thread-safe — concurrent parse() or format() calls corrupt internal state.

Solution

Replaced shared SimpleDateFormat with ThreadLocal<SimpleDateFormat>:

// Before (unsafe)
private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd");

// After (safe)
private static final ThreadLocal<SimpleDateFormat> DATE_FORMAT =
    ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd"));

// Usage: DATE_FORMAT.get().format(date);

File	Change
JSON.java (tqamds)	All date format instances made ThreadLocal

---

7. ThreadLocalRandom

Problem

Math.random() uses a shared Random instance with contention under concurrent access.

Solution

Replaced with ThreadLocalRandom.current().nextInt() / .nextDouble() where applicable.

---

8. AtomicBoolean Guards Against Concurrent Refresh

Problem

Scheduled data refresh tasks (GoGlobal, Rayna, Tiqets) had no guard against concurrent execution. If a refresh took longer than the interval, two refresh cycles could run simultaneously.

Solution

Added AtomicBoolean flag to prevent overlap:

private static final AtomicBoolean refreshing = new AtomicBoolean(false);

public void runRefresh() {
    if (!refreshing.compareAndSet(false, true)) {
        logger.info("Refresh already in progress — skipping");
        return;
    }
    try {
        doRefresh();
    } finally {
        refreshing.set(false);
    }
}

File	Guard
GGRefreshRunner.java (tqgglbl)	GoGlobal refresh
SDRefreshRunner.java (tqryb2b)	Rayna B2B refresh
TiqetsPlugin.java (tqtiqets)	Tiqets catalog refresh

---

9. Daemon Threads for Scheduled Executors

Problem

ScheduledExecutorService threads were non-daemon, preventing JVM shutdown when the main thread exits.

Solution

Used ThreadFactory with setDaemon(true):

ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(r -> {
    Thread t = new Thread(r, "goglobal-refresh");
    t.setDaemon(true);
    return t;
});

File	Thread Name
GoGlobalPlugin.java	goglobal-refresh
RaynaB2BActPlugin.java	rayna-refresh

---

10. Shared HTTP Client Instances

Problem

New HTTP client objects were created per request, wasting resources and preventing connection reuse.

Solution

Created shared, reusable client instances:

File	Change
GoGlobalSoapClient.java	Shared HttpClient instance
TiqetsHttpClient.java	Shared HttpClient instance

---

11. Copy-on-Write Cache Refresh

Problem

TiqetsCacheManager modified live cache maps during refresh. Readers accessing the cache during refresh could see incomplete data.

Solution

Build new maps in local variables, then atomically swap the reference:

// Build new cache in local variables
Map<String, TiqetsProduct> newProducts = new HashMap<>();
Map<String, TiqetsExperience> newExperiences = new HashMap<>();
// ... populate from DB ...

// Atomic swap (volatile reference)
this.productsCache = newProducts;
this.experiencesCache = newExperiences;

---

12. Atomic API Roles Reload

Problem

ApiRoleManager reloaded role mappings by clearing and repopulating the map. During the window between clear and repopulate, all authorization checks would fail.

Solution

Build a new Properties object, then atomically replace the reference:

Properties newRoles = new Properties();
newRoles.load(inputStream);
// Atomic swap
this.roles = newRoles;

---

13. Synchronized Critical Sections

Problem

Several business objects accessed shared mutable state without synchronization.

Solution

Added synchronized blocks to protect critical sections:

File	Protected Operation
CCachedCart.java	Cart item list modifications
CTransientCart.java	Cart item list modifications
CartFacade.java	Payment creation (replaced by distributed lock in cluster mode)
BookingRequestFacade.java	Booking confirmation (replaced by distributed lock in cluster mode)
OdooServiceFactory.java	Service initialization

---

14. RaynaCacheManager Refreshing Flag Removal

Problem

RaynaCacheManager used a plain boolean refreshing flag that was not thread-safe and was redundant given the distributed locking and AtomicBoolean guard.

Solution

Removed the flag. Refresh coordination now uses the distributed lock (cluster mode) or AtomicBoolean guard (local mode).

---

15. Static Final Constants

Problem

Constant values (strings, numbers, configuration keys) were declared as instance fields or non-final statics, creating unnecessary per-instance overhead and mutation risk.

Solution

Converted to static final constants across ~30 files. Examples:

// Before
private String LOCK_KEY = "GoGlobalRefresh";

// After
private static final String LOCK_KEY = "GoGlobalRefresh";

---

16. Static Final Logger Fields

Problem

Logger instances were created as instance fields (private Logger logger = ...), creating a new logger reference per object instance.

Solution

Changed to private static final Logger logger = ... across all affected files.

---

17. Defensive Copy of Properties

Problem

Properties objects were shared by reference, allowing callers to mutate configuration state.

Solution

Return defensive copies where properties are exposed to external callers.

---

18. Odoo userId Made Volatile

Problem

OdooServiceFactory.userId was set lazily on first use and read by multiple threads without synchronization.

Solution

Made the field volatile to ensure visibility across threads.

---

19. Bug Fix: Reference Comparison (== vs .equals())

Problem

EntityFacade compared entity action strings using == instead of .equals(), which would fail for dynamically constructed strings.

Solution

// Before (broken for non-interned strings)
if (action == "create") { ... }

// After (correct)
if ("create".equals(action)) { ... }

---

20. Immutable List from Arrays.asList Fixed

Problem

Arrays.asList() returns a fixed-size list that throws UnsupportedOperationException on add() or remove(). Code in EntityFacade was adding elements to such lists.

Solution

Wrapped in new ArrayList<>(Arrays.asList(...)) to create a mutable copy.

---

21. SQL Injection Prevention

Problem

NTSCruiseService constructed HQL queries by string concatenation with user-provided values.

Solution

Replaced with parameterized queries:

// Before (vulnerable)
String hql = "FROM CruiseEntity WHERE name = '" + name + "'";

// After (safe)
String hql = "FROM CruiseEntity WHERE name = :name";
query.setParameter("name", name);

---

22. StaticMapCache getEntry Graceful Handling

Problem

StaticMapCache.getEntry() could throw NullPointerException if the cache was invalidated between the null check and the map access.

Solution

Added null-safe access with graceful fallback.

---

23. Dead Code Removal

Problem

Unused methods and fields accumulated across the codebase.

Solution

Removed dead code:

File	Removed
NTSHotelFacade.java	Unused methods

---

24. Double-Confirmation Prevention for Bookings

Problem

BookingRequestFacade could process the same booking confirmation twice if two concurrent requests arrived (e.g., user double-clicks, or load balancer retry).

Solution

After acquiring the distributed lock, reload the booking status. If already confirmed, return the existing confirmation without making a duplicate supplier call:

boolean acquired = lockMap.tryLock("booking-confirm-" + bkReqId, 15, TimeUnit.SECONDS);
if (acquired) {
    try {
        // Reload status after lock acquisition
        BookingRequest reloaded = loadBookingRequest(bkReqId);
        if (reloaded.isConfirmed()) {
            return reloaded; // Already confirmed
        }
        // Proceed with supplier confirmation
        confirmWithSupplier(reloaded);
    } finally {
        lockMap.unlock("booking-confirm-" + bkReqId);
    }
}

---

Files Modified Summary

By Module

Module	Files	Key Changes
tqcommon	8	Volatile singletons, ConcurrentHashMap, EntityFacade bug fixes
tqapp	20	Session leak fixes, transaction rollback, synchronized sections, API roles
tqapi	4	Authentication filter, server initialization, correlation ID
tqamds	5	ThreadLocal DateFormat, session leak fixes, volatile singleton
tqryb2b	6	AtomicBoolean guard, daemon threads, session leak fixes
tqtiqets	5	Copy-on-write cache, AtomicBoolean guard, distributed invalidation
tqgglbl	5	AtomicBoolean guard, daemon threads, shared HTTP client
tqodoo	3	Volatile fields, ConcurrentHashMap, synchronized init

Impact Summary

Category	Count	Severity
Resource leaks (session/connection)	~50 methods	Critical
Thread-safety violations	~20 instances	High
Concurrency bugs (data corruption)	~10 instances	High
Security vulnerabilities (SQL injection)	1 instance	Critical
Logic bugs (==, immutable lists)	2 instances	High
Performance improvements	~8 instances	Medium
Code quality (constants, dead code)	~30 files	Low