HW "Asynchronous user request processing and caching"#250
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the asynchronous processing with CompletableFuture! The overall structure is well-implemented. However, changes are needed to address a critical race condition in your AsyncRequestProcessor.java file. The current if (!cache.containsKey(userId)) check followed by the supplyAsync block is not an atomic operation. This means if multiple threads request the same new user simultaneously, they can all pass the check and run the expensive getData method, defeating the purpose of the cache. To fix this, I recommend looking into the atomic methods provided by ConcurrentHashMap, specifically computeIfAbsent(). This will ensure the data processing logic is executed only once per user ID. Keep up the great work!
✨ 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
| if (cache.containsKey(userId)) { | ||
| return CompletableFuture.completedFuture(cache.get(userId)); | ||
| } | ||
| return CompletableFuture.supplyAsync(() -> { |
There was a problem hiding this comment.
This if check followed by a separate action inside the supplyAsync block creates a race condition. If two threads request the same userId that isn't in the cache yet, both can pass this check before the first one populates the cache. This results in the data processing task running twice for the same user.
To ensure atomicity (that the check and the action are a single, inseparable operation), you could use cache.computeIfAbsent(). This method guarantees that the value-producing lambda is only called once per key.
|
I corrected my job |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work resolving the race condition with computeIfAbsent—that's a great, thread-safe solution. However, I am requesting one change to fully meet the task's requirements. The task explicitly requires the cache to be of type Map<String, UserData>, but your current implementation in AsyncRequestProcessor.java uses Map<String, CompletableFuture<UserData>>. While your approach is a robust pattern, it deviates from this specific technical constraint. Please refactor your logic to use the correct cache type while still preventing the race condition.
✨ 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
| public class AsyncRequestProcessor { | ||
| private final Executor executor; | ||
|
|
||
| private final Map<String, CompletableFuture<UserData>> cache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
While this implementation correctly avoids the race condition, it deviates from the task requirement which specifies the cache should be of type Map<String, UserData>. It is possible to meet all requirements, including fixing the race condition, while using the specified cache type. Please adjust the cache to store UserData objects directly, not CompletableFutures.
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 task! I am approving your solution as it perfectly meets all the requirements. You've done a great job refactoring the caching logic to use Map<String, UserData>. Your use of computeIfAbsent is particularly impressive—it's an elegant and idiomatic way to ensure thread safety and prevent race conditions. The final implementation is clean, efficient, and robust. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.