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

[Fix] 캠퍼스팀 요구사항 반영 #509

Merged
merged 4 commits into from
Dec 19, 2024
Merged

[Fix] 캠퍼스팀 요구사항 반영 #509

merged 4 commits into from
Dec 19, 2024

Conversation

wateralsie
Copy link
Contributor

작업 내용

  • 공지사항 배너 ui 깨짐 이슈 해결
  • 식단 더보기 클릭, a/b테스트 로깅 수정 및 추가
  • a/b테스트 토큰 (access_history_id) 있는지 확인 후 실험군 조회하도록 로직 변경

@wateralsie wateralsie self-assigned this Dec 15, 2024
@wateralsie wateralsie requested a review from a team as a code owner December 15, 2024 15:00
@github-actions github-actions bot added the fix label Dec 15, 2024
Copy link
Collaborator

@Jokwanhee Jokwanhee left a comment

Choose a reason for hiding this comment

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

고생했습니다~ 커멘트 달아준거 확인함 해주세요! 🙇🏻


suspend operator fun invoke(title: String): Result<String> {
return runCatching {
mutex.withLock {
Copy link
Collaborator

Choose a reason for hiding this comment

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

뮤텍스로 동기화 문제를 고려해본 것이 너무 좋아보이네요!! 👍 (제가 저번주부터 코드 리뷰를 적극적으로 해보기 노력중입니다..)

보다보니 궁금한 부분이 생겨서 질문 남겨요!
해당 플로우에서는 Dispatcher.Main 에서 코루틴이 동작하는 것으로 보입니다. 그렇다고 하면 하나의 메인 스레드가 동작하는데 동기화 문제를 해결한다는 것이 조금 모순이지 않나 싶었어요. 여러 스레드에서 공유 자원을 사용하다보니 생길 수 있는 동기화 문제인데 이 부분은 하나의 스레드이잖아요?
그렇다면 위 코드에서 동기화 문제가 실제로 발생하는 지 궁금하네요..

만약 Dispatchers.IO 나 Dispatchers.Default 를 사용하여 스레드 풀에서 워커 스레드를 가져와 사용했다면 동기화 문제가 있을 수 있다고 생각이 듭니다.

간단한 예시로 아래 예시를 들어볼게요!
아래 결과는 얼마가 나올 것 같나요?

private var number = 0

fun main(): Unit = runBlocking {
    coroutineScope {
        repeat(100_000) {
            launch {
                number++
            }
        }
    }

    println("number : $number")
}

그리고 아래 예시를 보면서 Dispatchers.IO를 사용할 때는 어떨까요?

private var number = 0

fun main(): Unit = runBlocking {
    coroutineScope {
        repeat(100_000) {
            launch(Dispatchers.IO) {
                number++
            }
        }
    }

    println("number : $number")
}

결과를 확인해보면 알 수 있겠지만, IO를 사용할 경우에는 number의 값이 이상하죠? 동기화 문제가 발생한겁니다.
만약에 뮤텍스로 상호 배제를 보장하여 동기화 문제를 해결한다면 number의 값은 100000 이 나올 것입니다.

val mutex = Mutex()

...
launch(Dispatchers.IO) {
  mutex.withLock {
    number++
  }
}

그래서 이러한 부분을 고려해보니 해당 코드에서 궁금한 부분이 있었네요.
혹시 위 코드에서 동기화 문제가 있을 수 있다고 판단한 근거가 조금 궁금한데 여쭤봐도 괜찮을까요?
(+ 추가적으로 해당 동기화 문제에 대한 인사이트는 https://brunch.co.kr/@mystoryg/227 아티클을 읽어봤습니다 ☺️)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jokwanhee
정성스런 코드리뷰와 자세한 설명 감사합니다👍👍 덕분에 해당 문제에 대해서 제대로 고민해보게 되었습니다!

혹시 위 코드에서 동기화 문제가 있을 수 있다고 판단한 근거가 조금 궁금한데 여쭤봐도 괜찮을까요?

로컬에 access_history_id 토큰이 있을때만 AB테스트 실험군 조회하는 로직을 구현하고 있었는데, 화면당 실험군 개수만큼 토큰 api가 호출되더라고요. 아래 조건문에 동시에 접근되는 것이 원인이라고 판단했습니다

if (tokenRepository.getAccessHistoryId() == null) {

그리고 코드 다시 보니까 getAccessHistoryId()가 Dispatchers.IO를 쓰고 있었습니다! 원인을 드디어 찾았네용...

EventLogger.logClickEvent(
EventAction.CAMPUS,
EventLogger.logCustomEvent(
"AB_TEST",
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 부분도 EventAction.CAMPUS 처럼 EventAction.ABTEST로 바꿔보는건 어떨까요?
그나저나 이제 AB테스트 관련 로깅에는 해당 팀의 이름이 아닌 AB테스트로 표시하기로 바꾼건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hsgo2430
넵 모든 ab테스트 로깅은 문서대로 진행하고 있었습니다!

image

++ 반영해서 ab테스트 로깅 함수 함 만들어봤습니다 30fa3be

Copy link
Contributor

@ThirFir ThirFir left a comment

Choose a reason for hiding this comment

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

수고하셨어용 ㅎㅎ

@wateralsie wateralsie merged commit 4b07d5c into develop Dec 19, 2024
@wateralsie wateralsie deleted the fix/campus branch December 19, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants