Add Thread-Specific Storage design pattern #3422
Conversation
PR SummaryImplements the Thread-Specific Storage (thread-local) design pattern to provide per-thread isolation of shared data. Introduces Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 23f22a3: Add Thread-Specific Storage design pattern
Files Processed (13)
- pom.xml (1 hunk)
- thread-specific-storage/README.md (1 hunk)
- thread-specific-storage/etc/ThreadSpecificStorageUML.png (0 hunks)
- thread-specific-storage/etc/thread-specific-storage.urm.puml (1 hunk)
- thread-specific-storage/pom.xml (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContext.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContextProxy.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/RequestHandlerTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextProxyTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextTest.java (1 hunk)
Actionable Comments (2)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [27-27]
bug: "Logger usage inconsistent with Lombok setup"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextProxyTest.java [50-60]
possible bug: "Assertions inside worker thread won't fail the test reliably; capture exceptions and assert after join"
Skipped Comments (3)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContextProxy.java [18-18]
maintainability: "Static accessor methods suggest singleton-like usage; prefer instance-based API"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [23-37]
possible bug: "Test fragility: use thread join instead of sleep"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [31-33]
readability: "Test-check relies on logger output in stdout; ensure logback config prints to stdout or adjust test"
| * 4. Finally, clear thread-local to prevent memory leak. | ||
| */ | ||
| public void process() { | ||
| LOGGER.info("Start handling request with token: {}", token); |
There was a problem hiding this comment.
LOGGER is not defined when using Lombok @slf4j. Lombok generates a 'log' field. This will fail to compile. Replace with log.info(...).
| void testThreadIsolation() throws InterruptedException { | ||
| UserContext context1 = new UserContext(123L); | ||
| UserContext context2 = new UserContext(456L); | ||
| UserContextProxy.set(context1); | ||
| // Create another thread to set different context | ||
| Thread thread = new Thread(() -> { | ||
| UserContextProxy.set(context2); | ||
| UserContext threadContext = UserContextProxy.get(); | ||
| assertNotNull(threadContext); | ||
| assertEquals(456L, threadContext.getUserId()); | ||
| }); |
There was a problem hiding this comment.
Thread thread = new Thread(() -> {
UserContextProxy.set(context2);
UserContext threadContext = UserContextProxy.get();
assertNotNull(threadContext);
assertEquals(456L, threadContext.getUserId());
});
thread.start();
thread.join();
Run 'mvn spotless:apply' to fix these violations.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 5d8581a: fixed:pom.xml
Run 'mvn spotless:apply' to fix these violations.
Files Processed (8)
- pom.xml (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContextProxy.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/RequestHandlerTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextProxyTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextTest.java (1 hunk)
Actionable Comments (1)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [25-25]
best_practice: "Replace LOGGER with the Lombok-provided log"
Skipped Comments (3)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [38-38]
possible bug: "Possible NPE when retrieving from thread-local storage"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [33-34]
maintainability: "Incorrect System.out restoration in test"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [23-23]
enhancement: "Improve test synchronization"
| * clear thread-local to prevent memory leak. | ||
| */ | ||
| public void process() { | ||
| LOGGER.info("Start handling request with token: {}", token); |
There was a problem hiding this comment.
Use the Lombok-provided log field for SLF4J logging instead of the undefined LOGGER; Lombok's @slf4j exposes a 'log' instance.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 6cfccc0: Merge branch 'master' into Thread-Specific-Storage
Files Processed (1)
- pom.xml (1 hunk)
Actionable Comments (0)
Skipped Comments (5)
-
pom.xml [235-235]
maintainability: "Add module to multi-module build"
-
pom.xml [235-235]
readability: "Module naming and alignment"
-
pom.xml [235-235]
best_practice: "ThreadLocal cleanup and memory leaks"
-
pom.xml [235-235]
enhancement: "Tests coverage for thread scenarios"
-
pom.xml [235-235]
readability: "Documentation and README"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 34cc03f: fix: address Sonar security hotspots for Random usage and e.printStackTrace()
Files Processed (2)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java [41-41]
best practice: "Use Lombok-provided logger instance."
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [4-4]
maintainability: "Remove unused import."
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [26-26]
best practice: "Use Lombok-provided logger."
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [42-42]
best practice: "Use Lombok-provided logger."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 106f06d: fixed:
Run 'mvn spotless:apply'(remove unused import)
Files Processed (1)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
Actionable Comments (2)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [25-25]
best_practice: "Use Lombok-provided logger and redaction"
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [38-38]
possible bug: "Potential null dereference in thread-local access"
Skipped Comments (1)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [41-41]
best_practice: "Use Lombok-provided logger"
| * clear thread-local to prevent memory leak. | ||
| */ | ||
| public void process() { | ||
| LOGGER.info("Start handling request with token: {}", token); |
There was a problem hiding this comment.
Replace LOGGER with log to align with Lombok @slf4j; also avoid logging the raw token to prevent sensitive information exposure.
| Thread.sleep(200); | ||
|
|
||
| // Step 3: Retrieve userId later in the request flow | ||
| Long retrievedId = contextProxy.get().getUserId(); |
There was a problem hiding this comment.
contextProxy.get() may return null; guard against NullPointerException when retrieving userId.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- d3a8041: fixed:
Refactor Thread-Specific Storage pattern implementation to address Sonar issues: remove unnecessary instantiation, add private constructor, use diamond operator, and update tests.
Files Processed (6)
- thread-specific-storage/pom.xml (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContextProxy.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/RequestHandlerTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java [38-38]
bug: "Fix Lombok logging instance"
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [26-26]
readability: "Replace LOGGER with Lombok log and sanitize logs"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [17-18]
maintainability: "Restore System.out correctly in tests"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [23-28]
test: "Test stability for multi-thread output capturing"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (8)
- c110258: Trigger CI rerun
- 1bc4edd: fixed:
update uml and README.md - d3a8041: fixed:
Refactor Thread-Specific Storage pattern implementation to address Sonar issues: remove unnecessary instantiation, add private constructor, use diamond operator, and update tests. - 106f06d: fixed:
Run 'mvn spotless:apply'(remove unused import) - 34cc03f: fix: address Sonar security hotspots for Random usage and e.printStackTrace()
- 6cfccc0: Merge branch 'master' into Thread-Specific-Storage
- 5d8581a: fixed:pom.xml
Run 'mvn spotless:apply' to fix these violations. - 23f22a3: Add Thread-Specific Storage design pattern
Files Processed (13)
- pom.xml (1 hunk)
- thread-specific-storage/README.md (1 hunk)
- thread-specific-storage/etc/ThreadSpecificStorageUML.png (0 hunks)
- thread-specific-storage/etc/thread-specific-storage.urm.puml (1 hunk)
- thread-specific-storage/pom.xml (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContext.java (1 hunk)
- thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/UserContextProxy.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/RequestHandlerTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextProxyTest.java (1 hunk)
- thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/UserContextTest.java (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/RequestHandler.java [26-26]
best_practice: "Use Lombok-provided logger and avoid exposing token"
-
thread-specific-storage/src/main/java/com/iluwatar/threadspecificstorage/App.java [39-41]
best_practice: "Replace logger with Lombok's 'log' instance"
-
thread-specific-storage/src/test/java/com/iluwatar/threadspecificstorage/AppTest.java [21-27]
readability: "Testing sink mismatch: stdout capture vs. logging"
|
CI may fail due to the AI Reviewer workflow (missing LLM_API_KEY and required type field). |
|
|
Hi @iluwatar , This PR adds the Thread-Specific Storage design pattern implementation. Please review at your convenience. The PR is ready to merge. Thanks! 🙏 |
1 participant
What does this PR do?
This PR adds the Thread-Specific Storage design pattern implementation to the repository, addressing issue #3225.
Thread-Specific Storage is a concurrency design pattern where each thread retains its own isolated instance of shared data, preventing concurrency issues by eliminating the need for synchronization mechanisms.
Implementation Details:
UserContext: Represents the thread-specific object that stores user-related information (in this case, userId)UserContextProxy: Acts as a proxy that manages the ThreadLocal storage, providing methods to set, get, and clear thread-specific dataRequestHandler: Simulates an application thread that processes requests, demonstrating how thread-local storage can be used during request processingApp: Demonstrates the pattern with concurrent request processing for multiple usersKey Features:
ThreadLocal<T>to store per-thread data.Files Added:
src/main/java/com/iluwatar/threadspecificstorage/src/test/java/com/iluwatar/threadspecificstorage/pom.xmlThe implementation follows the repository's coding standards and includes thorough documentation and tests to verify behavior in multi-threaded scenarios.