feat: SSE 서비스 운영 상태 정의 로직 구현

[goldm0ng]

도입 배경

클라이언트는 SSE 연결 시 & SSE 연결 도중, Client는 서버가 실시간 여석을 정상적으로 제공하는지 판단할 수 없습니다.
따라서, 서비스 운영 상태를 SSE를 통해 전달하도록 합의했습니다.
자세한 내용은 노션 회의 자료 - 프백 회의를 참고해주세요!
https://www.notion.so/1c3acf7c316281b08678d870e3bcc5bf?p=25facf7c3162805fb4e1f0fafe46091c&pm=s

작업 내용

• SSE 서비스 운영 상태 정의하는 로직을 추가 구현하였습니다.

  • 서비스 운영 상태는 다음 두가지 상황에서 SSE로 추가로 전달됩니다.

  1. SSE Connection을 처음 맺을 때
  2. 서비스 운영 상태가 변경되었을 때
  

• 서비스 운영 상태는 다음 네 가지로 정의됩니다.

1. 기존, SseService에 서비스 운영 상태를 디폴트 값인 idle로 정의했습니다.

• 여기서, 타입을 volatile으로 한 이유는 스레드 동시성 이슈를 방지하고자 했습니다.

2. SSE Connection을 처음 맺을 때, sendStatusEvent 메서드를 통해 현재 서비스 운영 상태를 보냅니다.
3. updateStatus 메서드를 통해 서비스 운영 상태를 상황에 맞게 변경합니다. 상태가 바뀌면 현재 연결되어 있는 모든 클라이언트에게 SSE로 현재 서비스 운영 상태를 전송합니다.

• AdminPreseatService

  • preseat 크롤링 시 preseat 서비스 운영 상태로 변경합니다.
  • preseat 크롤링이 정상적으로 되지 않을 시 서비스 운영 상태를 error로 변경합니다.

• AdminSeatService

  • 여석 크롤링 시작 시 live 서비스 운영 상태로 변경합니다.
  • 여석 크롤링 중단 시 다시 디폴트 운영 상태인 idle로 변경합니다.
  • 여석 크롤링이 정상적으로 되지 않을 시 서비스 운영 상태를 error로 변경합니다.

고민 지점과 리뷰 포인트

서비스 운영 상태의 타입을 volatile로 정의하는 것이 적절한지에 대한 의견을 주시면 감사하겠습니다!!

[github-actions]

Test Results

114 tests   114 ✅  23s ⏱️
 29 suites    0 💤
 29 files      0 ❌

Results for commit 4bb883c.

♻️ This comment has been updated with latest results.

[3Juhwan]

이 PR에서 구현한 것들이 필요한 상황이 무엇인지 공유해 주실 수 있나요?? 서비스 운영 중에 관련 문제가 있었던 걸로 기억해요.

[3Juhwan]

이 기능은 운영에 꼭 필요한 것 같네요. 이로써 더 편리하게 서비스를 운영할 수 있겠어요.
좀 꼼꼼하게 코드 리뷰를 작성했어요.
코드를 잘 작성해 주었는데, 남긴 코멘트를 확인하면 변경사항이 좀더 발생할 수 있어요!

[3Juhwan]

이 메시지는 사용자에게 노출되는 건가요? 아니면 FE에게 제공하는 문구인가요?
사용자에게 직접 노출된다면 문구를 다듬어야 할 필요가 있겠어요.
우리 서비스를 처음 사용하는 사람도 이해할 수 있도록요!
예를 들면, 실시간 여석을 제공중이에요 -> 남은 자리를 실시간으로 볼 수 있어요 처럼요

[goldm0ng]

이 메시지는 사용자에게 노출되는 건가요? 아니면 FE에게 제공하는 문구인가요?

사용자에게 노출되는 정보입니다!

주환님 말씀대로 사용자에게 노출되는 정보인만큼 문구를 잘 다듬어야겠네요,,
preseat 여석이라는 용어는 사용자에게는 아주 생소한 용어일테니까요!
참고해서 수정해보도록 할게요!

[3Juhwan]

volatile을 사용한 이유가 무엇인지 궁금해요!
그리고 리뷰어 말랑씨 블로그에 관련 글이 있는데 매우 유익하네요!

[goldm0ng]

아주 유익한 말랑씨의 블로그 잘 봤습니다 :)

volatile을 사용한 가장 큰 이유는 동시성 이슈를 방지하고 싶었습니다!
현재 코드에서는 여러 스레드가 한 공유 변수를 사용하고 있는 구조입니다.
따라서 A라는 스레드와 B라는 스레드가 값을 읽고 쓸 때 다른 정보를 읽고 쓸 수가 있죠.
=> 문제 해결: volatile을 사용함으로써 여러 스레드가 최신 값을 볼 수 있도록 해줍니다.
다만, 캐시에 저장된 정보를 사용하지 않고 메인 메모리에서 직접 값을 읽고 쓰기 때문에 성능적으로는 좀 안 좋을 수 있겠네요 ㅠㅠ

volatile은 앞서 말씀드린대로 가시성을 보장해주지만, 원자성을 보장해주진 않습니다.
이로 인해 생길 수 있는 문제를 생각해보았습니다.

다음은 updateStatus 메서드의 코드입니다.

if (this.currentStatus != newStatus) {
    this.currentStatus = newStatus;
    propagateStatus();
}

=> 여러 스레드가 동시에 들어오면, 서로 같은 이전 상태를 보고 각각 propagateStatus()를 중복 호출할 수 있습니다.
따라서 클라이언트에 중복 전파가 발생할 수 있다는 점입니다.
이는 값 자체가 잘못되거나 하는 오류가 발생하는 건 아니지만, 최초 혹은 업데이트 시 상태 전파는 한 번만 하겠다는 의도가 깨져버릴 수 있습니다.

이를 해결하기 위해서는 가시성과 원자성을 모두 보장해주는 synchronized를 사용하는 방안이 있습니다.

---

정리하자면, 제가 volatile을 쓴 주목적은 가시성 보장입니다!
SSE 실시간 여석 데이터 제공 여부에 따른 상태는 빈번하게 바뀌지 않기 때문에 성능상으로도 문제가 크게 되지 않는다고 생각했어요.
지금 updateStatus 로직에서 중복 전파가 나더라도 데이터 일관성이 깨지는 건 아니고, 단지 같은 상태 전파가 여러 번 날아갈 수 있다 정도로 생각했습니당
따라서 저는 volatile 을 그대로 사용해도 괜찮을 것 같은데, 이에 대한 주환님의 의견이 궁금합니다!
혹시나 제가 잘못 알고 있는 내용이 있거나 놓친 부분이 있다면 알려주시면 감사하겠습니다 :)

[3Juhwan]

1. Early return 하는 것이 가독성에 좋아요.

Suggested change

	if (this.currentStatus != newStatus) {
	this.currentStatus = newStatus;
	propagateStatus();
	}
	if (this.currentStatus == newStatus) {
	return;
	}
	this.currentStatus = newStatus;
	propagateStatus();

2. 클래스 비교에는 equals 사용
   this.currentStatus와 newStatus가 enum이라 싱글톤이 보장되기에 동일성 비교 (==)도 가능하긴 한데요.
   그래도 클래스는 equals로 비교하는 것이 안정적이라 생각해요!
   나중에 enum 대신에 class로 변경된다면 위 로직은 모두 깨질테니깐요.

[goldm0ng]

1. 수정하겠습니다~!
2. null-safe한 Objects.equals로 비교하는 것으로 수정하도록 할게요!

[3Juhwan]

혹시 initialEvent를 처음 전송하는 이유를 아시나요?

만료 시간까지 아무런 데이터도 보내지 않으면 재연결 요청시 503 Service Unavailable 에러가 발생할 수 있습니다. 따라서 처음 SSE 연결 시 더미 데이터를 전달해주는 것이 안전합니다.
참고: https://tecoble.techcourse.co.kr/post/2022-10-11-server-sent-events/

그런데 항상 StatusEvent를 전송할거라면 initialEvent를 보내지 않아도 괜찮지 않을까 했어요. 잠깐 고민해보니, 스펙이 바뀜에 따라 StatusEvent를 전송하는 로직도 언젠가 사라질 수 있으니 initialEvent는 남겨두는 것이 좋겠어요.

[3Juhwan]

이 메서드들은 SseService를 처음 구현할 때에 철학과는 다소 다른 점이 있네요!
SseService는 확장 가능한 형태로 만들었어요. connect와 propagate 메서드만 있으면 SSE에서 필요한 웬만한 이벤트는 모두 처리할 수 있어요.

지금 PR처럼 sendStatusEvent, propagateStatus 같이 쓰임새가 한정되어 있는 메서드가 추가될수록 확장 가능한 형태에서 벗어난다고 생각해요. 또, 코드 중복도 생겼지요.

       // 코드 중복 
        sseEmitterStorage.getEmitters().forEach(emitter -> {
            SseEventBuilder eventBuilder = SseEventBuilderFactory.create("status", statusResponse);
            sendEvent(emitter, eventBuilder);
        });

이렇게 하기 보다는 sendStatusEvent 메서드를 사용하는 connect 메서드 내부에서 코드 작성하는 것이 좋겠어요.
SseEventBuilderFactory로 statusEventBuilder를 발행해서 sendEvent 하는 것이지요

[goldm0ng]

아하! 그런 의도가 있었군요.
말씀해주셔서 감사합니다!! 언급 주신대로 적용해보겠습니다~!

[3Juhwan]

이 PR에서 관리하려는 것은 실시간 여석 데이터를 서버가 제공할 수 있는 상태인가?로 이해했어요.
그런 측면에서 현재 코드의 SseStatus는 어색하게 느껴지네요! SseStatus는 말 그대로 SSE 상태를 의미한다고 느껴져요.

개인적으로 관리하려는 상태가 SseService에서 관리하는 것이 올바른가? 하는 생각이 들어요. SseService는 Sse연결과 데이터 전송을 담당하는 클래스에요. 실시간 여석 서비스가 어떻게 되든 관심 없는 클래스에요. 대신에 Admin 클래스에서 상태 관리하는 것이 객체의 책임 관점에서 더 명확하지 않을까요?

[goldm0ng]

이 PR에서 관리하려는 것은 실시간 여석 데이터를 서버가 제공할 수 있는 상태인가?로 이해했어요.

맥락을 잘 이해해주셨네요.
여기서 좀 더 부가 설명을 드리자면, 단순히 실시간 여석 데이터를 서버가 제공할 수 있냐? 아니냐? 를 관리하기 보다는
현재 진행되고 있는 서비스에 대한 상태 정보를 서버에서 관리하여, SSE event를 통해 클라이언트에게 실시간으로 정보를 전달하는 것이라고 이해하시면 됩니다!

[goldm0ng]

개인적으로 관리하려는 상태가 SseService에서 관리하는 것이 올바른가? 하는 생각이 들어요. SseService는 Sse연결과 데이터 전송을 담당하는 클래스에요. 실시간 여석 서비스가 어떻게 되든 관심 없는 클래스에요. 대신에 Admin 클래스에서 상태 관리하는 것이 객체의 책임 관점에서 더 명확하지 않을까요?

말씀하신 내용에 동의합니다!
'SSE로 상태를 전송한다.' 에서, SSE에 너무 치중해서 생각했던 것 같아요.
적절한 책임 분리를 위해서는 Admin 패키지 내에서 상태를 관리하는 것이 좋아보입니다!

[3Juhwan]

이 클래스도 마찬가지로, SseStatus라는 클래스명이 어색하죠. 의미는 Sse 상태인데, 내부에서 실질적으로 관리하는 상태는 실시간 여석과 관련되었으니깐요. 클래스 명을 바꾸고 다른 패키지로 이동하는 것이 좋겠습니다

[haeyoon1]

로직을 실행하기 전에 상태를 바꾸면, 이후 fetchPinSeat / fetchGeneralSeat 실행 중 오류가 발생해 실제 크롤링이 시작되지 않아도 상태가 LIVE로 바뀐다는 문제가 생길 것 같은데 상태를 바꾸는 시점을

        Credential credential = credentials.findByUserId(userId);
        fetchPinSeat(credential);
        fetchGeneralSeat(credential);
        sseService.updateStatus(SseStatus.LIVE);

이렇게 수정하는 것에 대해 어떻게 생각하시나요?

[goldm0ng]

로직이 실행 되었을 때 에러가 발생하면?

다음 코드와 같이,

1. 상태가 ERROR 상태로 업데이트 되고
2. SSE로 업데이트 된 상태(ERROR)가 propagate됩니다!

private void sendExternalRequestWithOutDetect(CrawlerSubject crawlerSubject, Credential credential) {
        try {
            log.info("[SeatService] [학교 서버] 요청 시도 과목: {}", crawlerSubject);
            SeatPayload requestPayload = SeatPayload.from(crawlerSubject);
            SeatResponse response = seatClient.execute(credential, requestPayload);
            CrawlerSeat renewedCrawlerSeat = createSeat(response, crawlerSubject);

            allSeatBuffer.add(
                ChangeSubjectsResponse.of(
                    crawlerSubject.getId(),
                    ChangeStatus.UPDATE, //의미없는 필드...
                    SeatUtils.getRemainSeat(renewedCrawlerSeat),
                    LocalDateTime.now()
                    //renewedCrawlerSeat.getCreatedAt() //추후 개션 예정
                )
            );

            synchronized (getSubjectLock(renewedCrawlerSeat.getId())) {
                seatPersistenceService.saveSeat(renewedCrawlerSeat);
            }

            lastSuccessCrawlingTime.updateAndGet(
                previousSuccessTime -> Math.max(previousSuccessTime, System.currentTimeMillis())
            );

        } catch (CrawlerAllcllException e) {
            log.error(
                "[여석] 외부 API 호출에 실패했습니다. 과목: " + crawlerSubject.getCuriNo() + "-" + crawlerSubject.getClassName());
            seatStreamStatusService.updateSeatStreamStatus(SeatStreamStatus.ERROR);
        }
    }

크롤링을 시작한다 == 서비스 운영을 시작한다 와 같은 의미이기 때문에 저 로직 순서가 맞다고 판단을 했는데, 다른 분들의 의견이 궁금하네요!!