Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c053f9b73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return lastActiveTimes.entrySet().stream() | ||
| .filter(entry -> entry.getValue().isAfter(gracePeriodThreshold)) | ||
| .map(Map.Entry::getKey) |
There was a problem hiding this comment.
Include active emitters when filtering by grace period
getUserTokens() now depends entirely on lastActiveTimes, but that timestamp is only set in add() and is never refreshed while an emitter stays connected; after 30 seconds, long-lived SSE sessions are excluded even though emitters still contains them. In that case, callers like ExternalService#getPinSubjects and PinSeatSender treat an actively connected user as inactive, so pin crawling and pin-seat pushes stop until the client reconnects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
저도 덱스님의 의견에 동의하는데요!
현재 로직대로라면, 30초 이상 살아있는 정상 SSE 연결이 비활성으로 처리될 수 있다고 생각합니다!
getUserTokens()가 lastActiveTimes만 기준으로 Grace Period 필터링을 하고 있는데, lastActiveTimes가 SSE 연결 시점에만 기록되고 이후 갱신되지 않기 때문에,
30초 이상 유지되는 정상 SSE 연결도 비활성으로 처리될 가능성이 있어보여요!
제가 이해한 흐름을 정리해보자면,
- SSE 연결 시 새 emitter가 생성되고
emitters와lastActiveTimes에 저장됩니다.- 크롤링 대상 선정을 위해 활성 토큰을 조회할 때 해당 메서드(
getUserTokens()) 가 호출됩니다.
그런데 getUserTokens()는 lastActiveTimes가 [now-30초] 이후인 토큰만 반환됩니다.
결과적으로, 연결이 계속 살아있어도 (즉, emitters에 존재해도) lastActiveTimes가 30초를 넘기면 목록에서 제외되지 않을까요??
혹시 제가 잘못 이해하고 있는 부분이 있다면 편하게 말씀해주시면 감사하겠습니다~!
Test Results182 tests 182 ✅ 25s ⏱️ Results for commit 8c53345. ♻️ This comment has been updated with latest results. |
goldm0ng
left a comment
There was a problem hiding this comment.
빠른 이슈 해결 감사합니다 진님! 🙇🏻♀️
덱스님 리뷰 방향성과 동일하게 몇 가지 리뷰 남겨보았는데 의견 부탁드립니다!!
수고 많으셨습니다~!
| return lastActiveTimes.entrySet().stream() | ||
| .filter(entry -> entry.getValue().isAfter(gracePeriodThreshold)) | ||
| .map(Map.Entry::getKey) |
There was a problem hiding this comment.
저도 덱스님의 의견에 동의하는데요!
현재 로직대로라면, 30초 이상 살아있는 정상 SSE 연결이 비활성으로 처리될 수 있다고 생각합니다!
getUserTokens()가 lastActiveTimes만 기준으로 Grace Period 필터링을 하고 있는데, lastActiveTimes가 SSE 연결 시점에만 기록되고 이후 갱신되지 않기 때문에,
30초 이상 유지되는 정상 SSE 연결도 비활성으로 처리될 가능성이 있어보여요!
제가 이해한 흐름을 정리해보자면,
- SSE 연결 시 새 emitter가 생성되고
emitters와lastActiveTimes에 저장됩니다.- 크롤링 대상 선정을 위해 활성 토큰을 조회할 때 해당 메서드(
getUserTokens()) 가 호출됩니다.
그런데 getUserTokens()는 lastActiveTimes가 [now-30초] 이후인 토큰만 반환됩니다.
결과적으로, 연결이 계속 살아있어도 (즉, emitters에 존재해도) lastActiveTimes가 30초를 넘기면 목록에서 제외되지 않을까요??
혹시 제가 잘못 이해하고 있는 부분이 있다면 편하게 말씀해주시면 감사하겠습니다~!
| lastActiveTimes.entrySet().removeIf(entry -> { | ||
| boolean shouldRemove = entry.getValue().isBefore(cleanupThreshold); | ||
| if (shouldRemove) { | ||
| log.debug("[SSE] Grace Period 초과로 토큰 정리: {}", entry.getKey()); | ||
| } | ||
| return shouldRemove; |
There was a problem hiding this comment.
이 부분도 위 코멘트와 동일한 맥락으로,
현재 연결 중인 토큰에 대해서는 고려하지 않는 것 같아 보여서요!!
지금 로직대로라면, lastActiveTimes가 add() 시점에만 기록되고 갱신되지 않기 때문에 30초 이상 유지되는 정상 SSE 연결도 cleanup 시점에 lastActiveTimes에서 제거될 수 있어 보입니다. 연결 자체는 emitters에 남아있더라도요!!
위 코멘트 내용이 맞다면, cleanup 역시 아래 조건을 함께 반영하는 게 더 자연스러울 것 같습니다~
조건1. Grace Period를 초과했고
조건2. 현재emitters에도 없는 토큰
인 경우에 만 정리하는 방향성에 대해 어떻게 생각하시나요??
There was a problem hiding this comment.
그거시 맞습니다!! 연결 중인 토큰을 고려하여 코드를 수정하였습니다~!
꼼꼼한 리뷰 감사합니다~
src/test/java/kr/allcll/backend/support/sse/SseEmitterStorageTest.java
Outdated
Show resolved
Hide resolved
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.web.servlet.mvc.method.annotation.SseEmitter; | ||
|
|
||
| class SseEmitterStorageTest { |
There was a problem hiding this comment.
테스트 보완에 필요한 케이스를 좀 더 생각해보았습니다!
-
오래 연결된 토큰이 제외되지 않는지
SSE가 30초 이상 정상적으로 유지되더라도, 현재 연결 중인 토큰은 활성 토큰 목록에서 제외되지 않아야 합니다. -
연결 종료 후
Grace Period적용
SSE 연결이 끊긴 직후라도, 끊긴 시점 기준Grace Period이내라면 해당 토큰이 활성 토큰으로 인지되어야 합니다.
적용이 필요하다고 생각되는 부분에 한해, 추가로 보완해주시면 감사하겠습니다~~!
There was a problem hiding this comment.
SseEmitterStorageTest에 테스트 시나리오를 작성해봤습니다!
1. 연결을 추가하면 사용자 토큰 목록에서 조회 가능해야 한다.
2. 여러 사용자가 동시에 연결되면 모든 사용자의 토큰이 목록에 나타나야 한다.
3. 방금 연결된 사용자는 정리 작업이 실행되더라도 제거되면 안 된다.
4. 현재 활성 연결 수는 실제 연결된 사용자 수와 동일해야 한다.
5. 존재하는 사용자 토큰으로 연결 조회 시 연결이 존재해야 한다.
6. 존재하지 않는 사용자 토큰으로 조회하면 아무 연결도 반환되지 않아야 한다.
7. 오래 유지된 연결이라도 실제로 아직 연결 상태라면 활성 사용자로 계속 취급되어야 한다. (1번)
8. 활성 연결들은 시간 경과 여부와 관계없이 모두 사용자 목록에 포함되어야 한다. (1번)
9. 사용자가 방금 연결을 종료했더라도 일정 시간 동안은 사용자 목록에 남아 있어야 한다. (2번)
10. 연결 종료 후 일정 시간이 지나면 사용자 목록에서 제거되어야 한다. (2번)
11. 정리 작업은 실제로 연결이 끊겼고 일정 시간이 지난 사용자만 제거해야 하며, 여전히 연결 중인 사용자는 제거되면 안 된다.
There was a problem hiding this comment.
고생 많으셨습니다!
말씀드린 부분 보완하신 거 확인했고, 테스트 보완까지 깔꼼하게 해주셨네요!!
하나 의견 여쭤보고 싶은 것이 있습니다!
제안: 안정성 측면에서
disconnect시emitter의 동일성 체크를 추가하는 방향은 어떨까요?
이런 케이스가 아주아주 드물 것 같지만,,
동일 token으로 재연결되어 새 emitter가 등록된 이후에, 이전 emitter의 콜백이 지연 실행되면서 현재 연결을 끊어버리는 상황이 발생할 여지가 있을까요???
(현재 private final Map<String, SseConnection> connections; 와 같은 맵 구조이고,
disconnectToken(token)이 token 기준으로 connection을 찾아 disconnect 하는 것으로 보여서요!!)
만약, 그런 경우가 발생할 여지가 있다면, disconnet 시 emitter 동일성 체크를 하고 삭제하는 방향은 어떤지 의견 들어보고싶슴니다~
와 생각치도 못한 부분인데요! 충분히 가능한 상황이라고 생각합니다~ disconnection을 진행할 때 참조 동일성 검사 이후, 일치할 때만 로직 수행하도록 수정하였습니다~!! 추가로 말씀해주신 시나리오를 반영해 테스트도 아래 케이스로 보강했습니다.
|
|
기존에도 remove를 관리하긴 했잖아요, 그래서 storage는 "활성사용자의 커넥션을 관리한다."가 목표였구, 그래서 다음과 같은 코드가 있었습니당 sseEmitter.onTimeout(() -> {
emitters.remove(token);
log.debug("[SSE] 연결이 타임아웃으로 종료되었습니다. 현재 연결 수: {}", emitters.size());
});
sseEmitter.onError(e -> {
emitters.remove(token);
});remove해주는 친구가 이 친구만 있던 시점에, 죽은사용자인데 그를 위해 크롤링을 해주던 낭비가 얼마나 될거라구 예측하셨나용???궁금합니다 이거에 대해 의견을 주장하기 전에 connection이 죽은 경우인데 connection으로 관리되던게 얼마나 많다고 생각하는지 궁금해졌어용 |
Grace Period 기반 SSE 크롤링 대상 유지 로직 구현
기존 동작 메커니즘 (AS-IS)
결과: 사용자는 핀을 고정했지만 크롤링이 안 되어 알림을 받지 못함
새로운 동작 메커니즘 (TO-BE)
구현 내용
1. SseEmitterStorage 개선
새로운 필드 추가
주요 메서드
getUserTokens(): Grace Period 내 활성화된 토큰 반환 (기존 인터페이스 유지)getUserTokensWithinGracePeriod(): Grace Period 로직 구현cleanupExpiredActiveTimes(): 오래된 엔트리 정리2. ExternalPreInvoker 개선
주기적 정리 스케줄러 추가
메모리 누수 방지를 위한 주기적 정리 작업
3. 테스트 코드 작성
SseEmitterStorageTest.java- 6개의 단위 테스트기대 효과