[FIX] 스케줄러 기반 SSE ping으로 idle 연결 끊김 방지#6
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSpring Boot 애플리케이션이 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/auctionnotification/notification/service/SseEmitterService.java (1)
45-71:⚠️ Potential issue | 🟡 Minor동일 emitter에 대한 concurrent
send()호출은 가능하지만 Spring의 동기화로 안전함.
send()와sendPing()이 모두@Async("notificationExecutorWithVT")로 비동기 실행되고@Scheduled(fixedRate = 30000)이 이전 실행 완료를 기다리지 않으므로, 동일한SseEmitter에 여러 virtual thread의 동시 접근이 가능합니다. 다만 Spring Framework의ResponseBodyEmitter#send()는 내부적으로synchronized메서드로 구현되어(Spring 4.2 SPR-13224 이후) SSE 프레임 인터리브를 방지하므로 클라이언트 파싱 이슈는 없습니다.주의할 점은 메시지 순서 보장이 없다는 것입니다. 동기화는 프레임 손상을 막지만 서로 다른 스레드의 이벤트들이 도착 순서와 다르게 클라이언트에 전달될 수 있으므로, 메시지 순서가 중요하다면 emitter 인스턴스에 대한 외부 동기화를 검토하세요.
부수적으로,
name("ping")대신 SSE 주석 라인(:\n\n) 형태 keep-alive는 클라이언트 리스너를 트리거하지 않아 keep-alive 용도로 더 적합합니다(선택 사항).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/example/auctionnotification/notification/service/SseEmitterService.java` around lines 45 - 71, SseEmitterService currently allows concurrent send(Long, NotificationResponse) and sendPing() calls on the same SseEmitter (both annotated `@Async`), which Spring's ResponseBodyEmitter#send synchronizes internally but does NOT guarantee message ordering; if ordering matters, add external per-emitter synchronization: create and manage a per-user lock (e.g., a ConcurrentHashMap<Long, Object> locks or a lightweight per-emitter mutex) and wrap all calls that use emitters.get(userId) / emitters.entrySet() sends in synchronized blocks on that lock (ensure send() and sendPing() both acquire the same lock for a given user), and remove/cleanup the lock when removing the emitter in error handling to avoid leaks; keep the existing IOException handling and emitter removal logic in send/sendPing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/example/auctionnotification/notification/service/SseEmitterService.java`:
- Around line 62-71: The sendPing method iterates emitters and only catches
IOException, which allows other runtime exceptions (e.g., IllegalStateException
thrown by SseEmitter.send when the emitter has already completed) to escape and
abort the whole ping loop; update sendPing to catch Exception (or at least
IllegalStateException in addition to IOException) around
entry.getValue().send(...) so a failure for one emitter doesn't stop pings for
others, and when removing the emitter call emitters.remove(entry.getKey(),
entry.getValue()); also include the caught exception object in the log.warn call
(pass e as a parameter) to preserve the stacktrace and aid debugging; reference:
sendPing, emitters, SseEmitter.send, IOException, IllegalStateException, and the
existing log.warn/log.error usage.
---
Outside diff comments:
In
`@src/main/java/com/example/auctionnotification/notification/service/SseEmitterService.java`:
- Around line 45-71: SseEmitterService currently allows concurrent send(Long,
NotificationResponse) and sendPing() calls on the same SseEmitter (both
annotated `@Async`), which Spring's ResponseBodyEmitter#send synchronizes
internally but does NOT guarantee message ordering; if ordering matters, add
external per-emitter synchronization: create and manage a per-user lock (e.g., a
ConcurrentHashMap<Long, Object> locks or a lightweight per-emitter mutex) and
wrap all calls that use emitters.get(userId) / emitters.entrySet() sends in
synchronized blocks on that lock (ensure send() and sendPing() both acquire the
same lock for a given user), and remove/cleanup the lock when removing the
emitter in error handling to avoid leaks; keep the existing IOException handling
and emitter removal logic in send/sendPing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bbd5b2a-c317-4981-94ba-7bfc842fe804
📒 Files selected for processing (2)
src/main/java/com/example/auctionnotification/AuctionNotificationApplication.javasrc/main/java/com/example/auctionnotification/notification/service/SseEmitterService.java
2 participants
📝 작업 내용
@Scheduled(fixedRate = 30000)으로 30초마다 ping 이벤트 전송✅ 체크리스트 (Checklist)
💬 기타 사항
중간 발표 때 튜터님이 말씀해주신 이슈 해결한 내용입니다!
Summary by CodeRabbit
신규 기능