task is done#249
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is well-structured and correctly uses CompletableFuture and an ExecutorService. However, changes are needed because of a critical race condition in AsyncRequestProcessor.java. The current logic of checking if a user is in the cache (if (cache.get(userId) == null)) and then performing the computation is not an atomic operation. This can lead to a scenario where two concurrent requests for the same new user both find the cache empty and execute the time-consuming operation, which undermines the purpose of caching. To fix this, please use an atomic operation like ConcurrentHashMap.computeIfAbsent. A common pattern to solve this is to cache the CompletableFuture<UserData> itself, which ensures the computation is only started once.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| UserData cachedUserData = cache.get(userId); | ||
| if (cachedUserData != null) { | ||
| return CompletableFuture.completedFuture(cachedUserData); | ||
| } | ||
|
|
||
| return CompletableFuture.supplyAsync(() -> { | ||
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Request processing was interrupted", e); | ||
| } | ||
|
|
||
| UserData userData = new UserData(userId, "Details for " + userId); | ||
| cache.put(userId, userData); | ||
| return userData; | ||
| }, executor); |
There was a problem hiding this comment.
This implementation contains a race condition. If two threads call processRequest for the same new userId at roughly the same time, both will likely find that the cache is empty (line 17) and proceed to execute the time-consuming operation inside supplyAsync. This results in the same data being computed twice, which negates the benefit of caching for concurrent requests. To ensure the operation is performed only once, the check for the key and the computation of the value should be an atomic operation. Consider exploring methods like computeIfAbsent on ConcurrentHashMap which are designed for this purpose.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution. You've perfectly addressed the critical race condition by using ConcurrentHashMap.computeIfAbsent and caching the CompletableFuture<UserData> itself, which is a robust and highly efficient approach. While this meant deviating from the requirement to declare the cache as Map<String, UserData>, it was the correct decision to properly solve the concurrency issue. Your implementation is clean, well-structured, and meets all functional requirements. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.