Conversation
📝 WalkthroughWalkthroughThis PR introduces parallelization to candidate extraction in MidPointAsyncUseCase using CompletableFuture's allOf pattern, replacing synchronous per-candidate processing. Additionally, station name normalization (removing trailing "역") is added to both MidPointAsyncUseCase and SeoulMetroRouteParser for consistent station matching. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/swyp/mingling/domain/meeting/service/MidPointAsyncUseCase.java (2)
93-119:⚠️ Potential issue | 🟠 MajorBlocking I/O on
ForkJoinPool.commonPool()can starve the JVM-wide shared pool.
CompletableFuture.supplyAsync(...)without an explicitExecutorruns onForkJoinPool.commonPool(), which has limited parallelism (typicallyRuntime.availableProcessors() - 1). The lambdas here perform blocking HTTP calls (subwayRouteService.getRoute). With 5 candidates × N participants, you can easily saturate the common pool and block unrelated async work across the application.Inject a dedicated
Executor(e.g., aThreadPoolTaskExecutorbean) and pass it as the second argument:CompletableFuture.supplyAsync(() -> { ... }, asyncExecutor)
112-116:⚠️ Potential issue | 🔴 CriticalOriginal concern is addressed, but inconsistent normalization implementations create a data mismatch issue.
Lines 114–115 do pass raw station names to
subwayRouteService.getRoute(), but the API handles normalization internally at lines 31–32 ofSubwayRouteService, so the concern about mismatched names in the API call is not an issue.However, verification reveals a critical problem: normalization logic is inconsistent across the codebase.
SubwayRouteService.normalizeStationName()has a special case that preserves "서울역" unchanged (line 121), whileMidPointAsyncUseCase.normalizeStationName()always removes the "역" suffix. This means when MidPointAsyncUseCase normalizes "서울역" to "서울" for comparison (line 99), but then passes raw "서울역" to the API (lines 114–115), the API normalizes it to "서울역", creating a data inconsistency. The same names are used inconsistently throughout the response objects.Align the normalization implementations so all code applies the same rules.
🤖 Fix all issues with AI agents
In
`@src/main/java/swyp/mingling/domain/meeting/service/MidPointAsyncUseCase.java`:
- Around line 294-305: The normalizeStationName(String stationName) method in
MidPointAsyncUseCase incorrectly strips the trailing "역" from "서울역" (causing
mismatches); update normalizeStationName to preserve "서울역" by adding a guard
like "서울역".equals(stationName) before trimming, or better, extract and reuse a
shared utility (e.g., a new StationNameUtils.normalize or the existing
SubwayRouteService helper) and call that from
MidPointAsyncUseCase.normalizeStationName to ensure consistent behavior with
SeoulMetroRouteParser/SubwayRouteService.
- Around line 162-167: The code currently calls allCandidates.join() in
MidPointAsyncUseCase which can block indefinitely if any
subwayRouteService.getRoute() future hangs; modify the completion wait to apply
a timeout (e.g., use CompletableFuture.orTimeout on allCandidates or call get
with a TimeUnit timeout) and handle the TimeoutException/CompletionException
path to fail the request gracefully or cancel lingering candidateFutures—update
references to allCandidates, candidateFutures and the
subwayRouteService.getRoute() callers so timed-out futures are logged/cancelled
and a proper error response is returned instead of blocking forever.
- Around line 136-154: The code in MidPointAsyncUseCase that computes
min/max/avg over "routes" must guard against an empty list: before computing
min/max/avg and creating a MidPointCandidate, add an early check like "if
(routes == null || routes.isEmpty())" and handle it (either return a safe value
— e.g., null or Optional.empty() — or throw a clear exception such as
IllegalArgumentException with a descriptive message); update the method's return
type/signature if you choose Optional to propagate the empty-case safely instead
of causing divide-by-zero and wrong min/max values when creating the
MidPointCandidate.
| int min = Integer.MAX_VALUE; | ||
| int max = Integer.MIN_VALUE; | ||
|
|
||
| int deviation = max - min; | ||
| for (SubwayRouteInfo route : routes) { | ||
| int time = route.getTotalTravelTime(); | ||
| min = Math.min(min, time); | ||
| max = Math.max(max, time); | ||
| } | ||
|
|
||
| int sum = 0; | ||
| for (SubwayRouteInfo route : routes) { | ||
| sum += route.getTotalTravelTime(); | ||
| } | ||
| int deviation = max - min; | ||
|
|
||
| int sum = 0; | ||
| for (SubwayRouteInfo route : routes) { | ||
| sum += route.getTotalTravelTime(); | ||
| } | ||
|
|
||
| int avgTime = sum / routes.size(); | ||
|
|
||
| return new MidPointCandidate(routes, deviation, avgTime); |
There was a problem hiding this comment.
Edge case: routes being empty would cause ArithmeticException on division and incorrect min/max.
If departurelists is empty, routes will be an empty list. min stays Integer.MAX_VALUE, max stays Integer.MIN_VALUE, deviation overflows, and line 152 divides by zero. Consider adding a guard.
💡 Suggested guard
+ if (routes.isEmpty()) {
+ return new MidPointCandidate(routes, Integer.MAX_VALUE, Integer.MAX_VALUE);
+ }
+
int min = Integer.MAX_VALUE;
int max = Integer.MIN_VALUE;🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/domain/meeting/service/MidPointAsyncUseCase.java`
around lines 136 - 154, The code in MidPointAsyncUseCase that computes
min/max/avg over "routes" must guard against an empty list: before computing
min/max/avg and creating a MidPointCandidate, add an early check like "if
(routes == null || routes.isEmpty())" and handle it (either return a safe value
— e.g., null or Optional.empty() — or throw a clear exception such as
IllegalArgumentException with a descriptive message); update the method's return
type/signature if you choose Optional to propagate the empty-case safely instead
of causing divide-by-zero and wrong min/max values when creating the
MidPointCandidate.
| CompletableFuture<Void> allCandidates = CompletableFuture.allOf( | ||
| candidateFutures.toArray(new CompletableFuture[0]) | ||
| ); | ||
|
|
||
| return new MidPointCandidate(routes, deviation, avgTime); | ||
| })) | ||
| .toList(); // 객체를 하나의 리스트로 생성 | ||
| // 모든 작업 완료 대기 후 결과 수집 | ||
| allCandidates.join(); |
There was a problem hiding this comment.
No timeout — a hung API call will block the request indefinitely.
allCandidates.join() at line 167 has no timeout. If any subwayRouteService.getRoute() call hangs or is extremely slow, this will block the calling thread (and the HTTP request) forever.
Consider using orTimeout or get with a timeout:
💡 Proposed fix
- allCandidates.join();
+ try {
+ allCandidates.get(30, java.util.concurrent.TimeUnit.SECONDS);
+ } catch (Exception e) {
+ log.error("중간지점 계산 타임아웃 - meetingId: {}", meetingId, e);
+ throw new RuntimeException("중간지점 계산 시간 초과", e);
+ }Or if on Java 9+:
allCandidates.orTimeout(30, TimeUnit.SECONDS).join();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CompletableFuture<Void> allCandidates = CompletableFuture.allOf( | |
| candidateFutures.toArray(new CompletableFuture[0]) | |
| ); | |
| return new MidPointCandidate(routes, deviation, avgTime); | |
| })) | |
| .toList(); // 객체를 하나의 리스트로 생성 | |
| // 모든 작업 완료 대기 후 결과 수집 | |
| allCandidates.join(); | |
| CompletableFuture<Void> allCandidates = CompletableFuture.allOf( | |
| candidateFutures.toArray(new CompletableFuture[0]) | |
| ); | |
| // 모든 작업 완료 대기 후 결과 수집 | |
| try { | |
| allCandidates.get(30, java.util.concurrent.TimeUnit.SECONDS); | |
| } catch (Exception e) { | |
| log.error("중간지점 계산 타임아웃 - meetingId: {}", meetingId, e); | |
| throw new RuntimeException("중간지점 계산 시간 초과", e); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/domain/meeting/service/MidPointAsyncUseCase.java`
around lines 162 - 167, The code currently calls allCandidates.join() in
MidPointAsyncUseCase which can block indefinitely if any
subwayRouteService.getRoute() future hangs; modify the completion wait to apply
a timeout (e.g., use CompletableFuture.orTimeout on allCandidates or call get
with a TimeUnit timeout) and handle the TimeoutException/CompletionException
path to fail the request gracefully or cancel lingering candidateFutures—update
references to allCandidates, candidateFutures and the
subwayRouteService.getRoute() callers so timed-out futures are logged/cancelled
and a proper error response is returned instead of blocking forever.
| private String normalizeStationName(String stationName) { | ||
| if (stationName == null || stationName.isEmpty()) { | ||
| return stationName; | ||
| } | ||
|
|
||
| // "역"이 끝에 붙어있으면 제거 | ||
| if (stationName.endsWith("역")) { | ||
| return stationName.substring(0, stationName.length() - 1); | ||
| } | ||
|
|
||
| return stationName; | ||
| } |
There was a problem hiding this comment.
Bug: Same missing "서울역" special case as in SeoulMetroRouteParser.
This copy also lacks the "서울역" guard present in SubwayRouteService. "서울역" will be incorrectly shortened to "서울", breaking the equality check at line 99 and any downstream lookups.
Apply the same fix (add the "서울역".equals(stationName) guard), or better yet, share a single utility method.
🤖 Prompt for AI Agents
In `@src/main/java/swyp/mingling/domain/meeting/service/MidPointAsyncUseCase.java`
around lines 294 - 305, The normalizeStationName(String stationName) method in
MidPointAsyncUseCase incorrectly strips the trailing "역" from "서울역" (causing
mismatches); update normalizeStationName to preserve "서울역" by adding a guard
like "서울역".equals(stationName) before trimming, or better, extract and reuse a
shared utility (e.g., a new StationNameUtils.normalize or the existing
SubwayRouteService helper) and call that from
MidPointAsyncUseCase.normalizeStationName to ensure consistent behavior with
SeoulMetroRouteParser/SubwayRouteService.
2 participants
PR 제목
작업 유형
작업 내용
체크리스트
참고 사항 (선택)
Summary by CodeRabbit
Release Notes
Performance
Bug Fixes