-
Notifications
You must be signed in to change notification settings - Fork 1
[Koin Project][Refactor] 분실물 채팅 리팩토링 #1184
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
base: develop
Are you sure you want to change the base?
Conversation
kongwoojin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다
코멘트 확인해주세요
| minSendPeriod = 30000.milliseconds, // Follow backend recommendation | ||
| expectedPeriod = 30000.milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30.seconds
를 사용해도 될 것 같습니다
| Timber.w("WebSocketException: attempting reconnect...") | ||
| disconnect() | ||
| connect(retry = true) | ||
| val delayTime = (1000L * attempt.coerceAtMost(5)).coerceAtMost(16000L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 되면 최대 delayTime이 1000L * 5라 5000L이지 않나요?
마지막의 coerceAtMost(16000L)은 불필요해보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 전 노안이 와서 확인을 제대로 못했네요
| disconnect() | ||
| connect(retry = true) | ||
| val delayTime = (1000L * attempt.coerceAtMost(5)).coerceAtMost(16000L) | ||
| delay(delayTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delay를 제가 주지 않았던 이유는... 만약 reconnect 과정 중 delay가 걸린 상태에서 사용자가 send 요청을 보낸다면 해당 메시지가 씹히거나 에러가 발생될 수 있다고 생각해서였습니다.
따라서, 만약 delay를 준다면 delay 중 사용자가 보낸 메시지에 대한 처리가 필요할 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다! 그부분 고려해서 추가적으로 보완한 로직을 추후에 pr 올리려고 했는데 지금 pr로만 보니 불안전한 코드로 보이네요! 요부분은 따로 pr 분리해서 올리겠습니다! draft하고 작업해놓겠습니다!
| try { | ||
| jsonStompSession.convertAndSend(StompSendHeaders(headers), body, serializer) | ||
| } catch (e: UninitializedPropertyAccessException) { | ||
| throw e | ||
| } | ||
| jsonStompSession.convertAndSend(StompSendHeaders(headers), body, serializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 try-catch는 필요하지 않나요?
stomp 연결이 완료된 후 jsonStompSession가 초기화 되는데, 백엔드 서버 상태도 안 좋고, 이런저런 이유로 jsonStompSession가 초기화가 늦는 경우가 많아 try-catch로 처리 해뒀습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외를 잡아서 다시 throw만 하는 useless catch 아닌가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어 그러게요 이제 보니 왜 그냥 throw만한거지...?
PR 개요
이슈 번호: 1183
PR 체크리스트
작업사항
작업사항의 상세한 설명
heartbeat 주기 변경
heartbeat 주기를 4초에서 30초로 변경했습니다. 너무 짧은 주기는 모바일 환경에서 크래시를 발생시킬 수 있습니다. 웹은 10초, 모바일은 최소 20초 이상이 권장됩니다.
무한 루프 로직 변경
subscribe()의 재연결 로직을 무한 루프 방식에서 retryWhen + 지수 백오프로 개선했습니다. 1초부터 최대 16초까지 점진적으로 재시도하여 네트워크 불안정 상황에서도 안정적인 재연결을 보장하도록 구현했습니다.
채널 종료 예외 처리
ClosedReceiveChannelException 예외를 처리 추가해서 채널 종료 예외도 처리되도록 수정했습니다. 기존에 백그라운드에 오래 있었다가 다시 돌아왓을 때(채널 끊김) 크래시 나던 문제를 해결했습니다.
runBlocking 제거
추가로 토큰 주입할 때 runBlocking을 제거했습니다. runBlocking 사용한 부분이 너무 많아서 일단 저부분만 수정했습니다.
논의 사항
채팅은 처음 건들여 보는거라 제대로 작성 했는지 모르겠네욤
기록용 노션 끄적여봤는데 잘못된 내용 있으면 피드백 부탁드립니다🙇♀️
추가내용