Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] SSE 알림 서비스 구현 #122

Merged
merged 5 commits into from
Sep 1, 2024

Conversation

AppleMint98
Copy link
Collaborator

#️⃣연관된 이슈

ex) #117


📝작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

기본흐름

  1. 클라이언트에서 SSE 연결 요청을 보낸다.
  2. 서버에서는 클라이언트와 매핑되는 SSE 통신 객체를 만든다.
  3. 서버에서 이벤트가 발생하면 해당 객체를 통해 클라이언트로 데이터를 전달한다.
  • SseController 클래스

    • /connect 엔드포인트에서, SseEmitter 객체 생성
    • 이후 emitter.send() 메서드 호출 시 해당 엔드포인트에 name: value 필드 형식으로 반환
  • SseEmitters 클래스

    • sseEmitter 객체 서버저장용

스크린샷 (선택)

image


💬리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

로컬에서 노래 생성을 테스트 할 수 없어서, /api/home API 호출 시 해당 알림이 오도록 테스트했습니다

@AppleMint98 AppleMint98 added the ✨ Feature 기능 개발 label Aug 29, 2024
@AppleMint98 AppleMint98 self-assigned this Aug 29, 2024
Copy link
Collaborator

@JGoo99 JGoo99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

알림구현 맡아서 해주셔서 감사합니다ㅎㅎ 리뷰 확인 부탁드립니다!!

스크린샷보니까 data 부분에 인코딩 오류가 있는 것 같은데 charset 설정이 추가로 필요한가요??

(추가) 로컬환경 말고 blue, green 환경에서도 테스트했을 때 문제없었나요? Nginx 관련 이슈가 있어서 여쭤봅니당

Comment on lines 23 to 24
public SseEmitter connect() {
SseEmitter emitter = new SseEmitter(10 * 60 * 1000L);
Copy link
Collaborator

@JGoo99 JGoo99 Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

연결 시에 회원 식별값에 대한 정보 없이도 해당 기기로 알림이 갈 수 있나요??

음악 생성이 비동기로 진행되기 때문에 여러 구독이 발생할 수 있고,
구독된 이벤트 중 특정 이벤트만 발생시켜야 한다면 식별정보가 필요할 것 같아서 여쭤봅니다!

만약 식별정보를 추가한다면 ConcurrentHashMap 같은 것도 좋을 것 같아요. (key : memberId, value: SseEmitter)

Comment on lines 21 to 29
emitter.onCompletion(() -> {
log.info("onCompletion callback");
this.emitters.remove(emitter);
});

emitter.onTimeout(() -> {
log.info("onTimeout callback");
emitter.complete();
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onCompletion, onTimeout 이랑 추가로 onError 핸들러를 호출한 이후에
this.emitters.add(emitter); 를 마지막에 호출하는 건 어떤가요?


그리고 onTimeout 핸들러에서는 emitters.remove 가 아닌 emitter.complete() 를 호출하셨는데,
이유가 아래 흐름대로 동작하기를 유도하신 건가요?? 이 방식대로 흘러가는게 맞는지도 궁금합니다..!!

onTimeout 핸들러 호출 -> emitter.complete() -> onCompletion 핸들러 호출 -> this.emitters.remove(emitter);

Comment on lines 35 to 40
emitters.forEach(emitter -> {
try {
emitter.send(SseEmitter.event()
.name("notification")
.data(message));
log.info("Notification sent: {}", message);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음악 생성의 경우 알림 1회 전송이 끝인데, 알림 구독을 취소하는(emitters 맵에서 삭제하는) 로직은 없어도 되나요??

만약 구독 취소를 구현해야 한다면 유지보수를 위해서 따로 구독취소 컨트롤러를 생성하면 좋을 것 같아요. (n회 알림이 필요한 경우 수정하지 않아도되기 때문에)

@AppleMint98
Copy link
Collaborator Author

질문

  • 배포환경에서의 음악 생성 시 테스트를 해보고싶은데, 머지하지 않고 어떻게 확인할 수 있을까요...?

코멘트 수정사항

  1. ConcurrentHashMap으로 식별정보 추가했습니다. MemberId, SseEmitter를 Key, Value로 받아옵니다.

  2. onError() 추가했습니다. 연결 중 에러가 나면 호출되는 콜백 메서드 입니다

  3. 순서와 emitters.complete() 메서드는 구글링을 하면서 이렇게 사용하셔서 그대로 사용했는데, 고려해보지 못했습니다!
    emitters.put() 메서드의 위치별 차이점은 구글링으로 도저히 안나와서, GPT 답변을 받아왔는데 링크 첨부하겠습니다!
    https://ballistic-sceptre-a09.notion.site/SseEmitter-b347fde25945439989363b8ef8a92265?pvs=4
    혹시 관련 링크를 찾게되신다면 첨부 부탁드립니다!
    emitters.complete() 의 경우는, 이해하신 흐름이 맞고 complete()의 경우에는 단순히 remove()로 지우는 게 아니라
    모든 이벤트가 전송되었는지 확인한 다음 연결을 종료한다고 합니당
    https://stackoverflow.com/questions/55287474/should-spring-sseemitter-complete-trigger-an-eventsource-reconnect-how-to-cl
    덕분에 알아갑니다 감사합니다!!

  4. finally로 send 이후에 무조건 emitter를 삭제하도록 로직을 변경하였습니다.

변경 사항

  1. SseEmitters 클래스 삭제 -> SseService 클래스 변경
    SseEmitters 보다는, 해당과정을 Service로 분류하는게 더 일관성있을것같아서 변경했습니다.
    로직은 거의 동일하며, 컨트롤러에서의 로직도 서비스로 옮겼습니다.

  2. disconnect 메서드 추가 -> 단순 URL 호출로 SseEmitter를 지우는 메서드입니다.

@AppleMint98 AppleMint98 requested a review from JGoo99 August 30, 2024 13:31
Copy link
Collaborator

@JGoo99 JGoo99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배포환경에서 테스트 해보고싶었는데 잘 안 되네요..!

일단 nginx 에 설정 추가해뒀으니
소셜 로그인 후에 임의로 sse 구독하고, 아기 등록 후에 로그인이 완료되었다는 알림을 전송하는 코드를 추가한 뒤에 실제 배포환경에서 한 번 테스트해보는 것도 좋을 것 같아요!

Comment on lines 17 to 20
@RequiredArgsConstructor
public class SseService {

private final ConcurrentHashMap<Long, SseEmitter> emitters = new ConcurrentHashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자 주입 어노테이션은 없어도 될 것 같습니당

Comment on lines 54 to 68
public void sendOneNotification(Long memberId, String message) {
SseEmitter emitter = emitters.get(memberId);
log.info("memberId : {} , sendNotification", memberId);
if (emitter != null) {
try {
emitter.send(SseEmitter.event()
.name("notification")
.data(message, MediaType.TEXT_PLAIN));
} catch (IOException e) {
emitters.remove(memberId);
} finally {
emitters.remove(memberId);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1회 알림을 위해 따로 sendOneNotification 메서드를 구현하는 것 보다

기존처럼 다회 알림까지 가능한 sendNotification 으로 구현해두고

프론트 측에서 disconnect 엔드포인트 요청을 통해서 알림 구독을 해제하도록 하는 방식으로도 충분할 것 같아요.


private final SseService sseService;

@GetMapping(value = "/sse/connect", produces = MediaType.TEXT_EVENT_STREAM_VALUE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공통 urisserequestMapping 으로 분리해도 될 것 같아요!

Comment on lines 58 to 63
try {
emitter.send(SseEmitter.event()
.name("notification")
.data(message, MediaType.TEXT_PLAIN));
} catch (IOException e) {
emitters.remove(memberId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOExceptioin 발생 시 추가적인 처리는 필요없나요?? (만약 없다면 log.error 하나 찍어주세요!)

@AppleMint98 AppleMint98 merged commit 7245f97 into KUIT-PROJECT:develop Sep 1, 2024
@AppleMint98 AppleMint98 deleted the Alarm branch September 1, 2024 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants