-
Notifications
You must be signed in to change notification settings - Fork 0
[#292] 경고 알럿 표시 여부와 그 내용을 개선한다. #302
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
Conversation
YunDaeHyeon
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.
📄 PR 코드 리뷰 리포트
🔍 요약
이번 PR은 주로 연결 관리 및 상태 처리를 개선하고 불필요한 UI 상태를 제거하는 데 중점을 두었습니다. heartBeat 로깅을 줄이거나 Toast 컴포넌트의 바인딩을 명확히 하는 등 긍정적인 변경사항이 많습니다. 하지만 AsyncStream 사용 방식과 연결 해제 로직에서 몇 가지 중대한 문제가 발견되어 프로덕션 환경에서 연결 안정성과 이벤트 누락을 야기할 수 있습니다. 특히 스트림을 매번 새로 생성하는 방식은 AsyncStream의 기본 목적에 맞지 않으며, 심장 박동 타임아웃 시 불필요한 전체 연결 해제는 실제 사용자 경험에 문제를 일으킬 수 있습니다.
⚠️ 주요 문제
- 스트림 관리 문제 (AsyncStream 재활용 오류):
BrowserStreamManager에서browsingEventStream과connectionCheckHeartbeatStream이 계산 속성으로 정의되어 매번 접근할 때마다 새로운AsyncStream을 생성하고 이전Continuation을finish()하고 있습니다. 이 방식은AsyncStream의 설계 의도와 맞지 않으며, 이벤트가 구독자에게 전달되지 않고 유실되거나, 예상치 못한 동시성 문제 및 경쟁 조건을 유발할 수 있습니다.AsyncStream은 일반적으로 한 번 생성되어 여러 구독자가 공유하는 방식으로 사용되어야 합니다. - 심장 박동 타임아웃 시 과도한 연결 해제:
BrowsingStore의setupHeartbeatListener에서heartbeatTimeout또는remoteHeartbeatTimeout이 발생했을 때cleanupConnectedDevices()를 호출하여 미러링 기기와 리모트 기기 모두의 연결을 해제하고 있습니다. 미러링 기기의 심장 박동만 끊겼을 때 리모트 기기 연결까지 해제하는 것은 잘못된 동작입니다. 각 타임아웃 이벤트에 맞춰 해당 기기만 정리하도록 로직을 수정해야 합니다. stopHeartBeat명령의 재귀적 전송:BrowserCommandManager에서receivedCommand(.stopHeartBeat)를 처리할 때,delegate?.sendRemoteCommand(.stopHeartBeat)를 다시 호출하고 있습니다. 이 코드는stopHeartBeat명령을 수신하자마자 동일한 명령을 원격으로 다시 전송하려는 시도로, 무한 루프 또는 불필요한 네트워크 트래픽을 유발할 수 있습니다. 수신한 명령은 처리만 하고 다시 보내지 않아야 합니다.handleBrowserEvent(.deviceLost)의 잠재적 중복 동작:BrowsingStore의handleBrowserEvent에서deviceLost이벤트 발생 시browser.disconnect(useType:)를 명시적으로 호출하고 있습니다.deviceLost이벤트 자체가Browser내부에서 기기가 이미 연결 해제되어 발생한 것이라면, 여기서browser.disconnect()를 다시 호출하는 것은 중복일 수 있으며 예상치 못한 부작용을 일으킬 가능성이 있습니다.browser.disconnect()의 계약과 이벤트 발생 원인을 명확히 하여 중복 호출을 피해야 합니다.
🛠 개선 제안
BrowserStreamManager의yield메서드 안전성 강화:browsingEventContinuation과connectionCheckHeartbeatContinuation이 옵셔널이 되었으므로,yieldBrowsingEvent및yieldHeartbeatTimeoutToAll내부에서yield호출 전에 해당Continuation이 유효한지 확인하는guard let구문 등을 추가하여nil인 경우 이벤트를 무시할지 혹은 오류를 기록할지 명시적으로 처리하는 것이 좋습니다. 현재는?.yield를 사용하고 있어 이벤트가 자동으로 무시됩니다. 이는 스트림 관리 문제 해결 후 자연스럽게 해결될 부분입니다.BrowsingStore의cleanupConnectedDevices()세분화: 현재는 모든 연결을 해제하지만, 미러링 기기만 해제하거나 리모트 기기만 해제하는 등의 세분화된 정리 메서드를 추가하여setupHeartbeatListener등의 로직에서 필요한 부분만 선택적으로 정리할 수 있도록 하면 더 유연하고 정확한 상태 관리가 가능합니다.
✅ 긍정적 관찰
- 불필요한 UI 상태 제거:
showMirroringDisconnectedAlert및showHomeAlert상태 변수가 제거되어 코드 복잡성이 줄어들었습니다. - 로그 과도화 방지:
Browser.swift에서heartBeat명령은 로깅하지 않도록 하여 불필요한 로그 생성을 줄였습니다. Toast바인딩 일관성 개선:Toast+.swift에서onDisappear시isPresented바인딩을false로 명시적으로 설정하여 상태 불일치 문제를 방지했습니다.- 리스너 등록 시점 개선:
ConnectionCheckStore에서setupHeartbeatListener()호출 시점을action(.entry)에서init으로 옮겨, 리스너가 더 일찍 등록되도록 하여 잠재적인 타이밍 문제를 해결했습니다. - 명시적인 연결 해제 처리:
AdvertisingView에서 탭 제스처 시 연결되어 있는 경우disconnect인텐트를 보내도록 하여 연결 관리가 명시적으로 이루어지도록 개선했습니다. remoteHeartBeater해제 강화:Browser에서remoteHeartBeater가nil로 설정되도록 하여 리소스 관리를 개선했습니다.stopHeartBeat명령 처리 추가:AdvertiserCommandManager에서stopHeartBeat명령에 대한 처리 로직을 추가하여 새로운 기능을 지원합니다.
68f6835 to
85405e8
Compare
GRJeon
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.
실행해봤을 때는 잘 되는 것 같네요. 수많은 컨플릭트만 잘 해결할 수 있다면...
YunDaeHyeon
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.
너무 고생하셨습니다!!!
👍👍👍👍👍👍
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.
느린 심 장박동
opficdev
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.
커밋 하나하나 다 봤는데.. 작업량이 ㅠㅠ
고생하셨습니다
- Browser는 계산 프로퍼티로 스트림을 노출 - HeartBeeater는 스트림 매니저의 yield 메서드 사용 - 테스트 코드 모두 통과로 동등성 확인 완료
- MCNearbyServiceBrowserDelegate 분리 - discoveredPeers 상태 관리 분리 - Browser는 browsingManager 인스턴스를 사용하도록 개선 - delegate extenstion 제거
- executeCommand 메서드를 CommandManager의 execute(data:)로 분리 - BrowserCommandDelegate Protocol 정의 - Browser에서 delegate 채택
- 기존 Browser.MirroringDeviceCommand, Browser.RemoteDeviceCommand 사용처에서 Browser. 만 제거 - BrowserManager 폴더 생성 및 관련 파일 이동
4c2398f to
f33fbf0
Compare
🔗 연관된 이슈
📝 작업 내용
📌 요약
🔍 상세
BrowsingView에서 연결된 기기가 사라졌을 때, 선택 상태가 업데이트 되지 않아 다시 기기를 선택하지 못한다.BrowsingView에서 연결 끊김 알럿 제거 → 화면에 연결 상황이 잘 보여서 생략하는 편이 더 자연스러움BrowsingView에서처음부터 연결하기를 눌렀을 때, 연결이 끊긴 기기가 늦게 사라진다.ConnectionCheckView에서 다른 기기의 연결 끊김을 감지하지 못한다.ConnectionCheckView에서 다른 기기의 연결이 끊겨BrowsingView로 돌아갔을 때 끊긴 기기들이 남아있다.BrowsingView와ConnectionCheckView에 재진입 했을 때 Stream에 반응하지 못한다 (예: 새로운 기기 감지가 전달되지 않음)ConnectionCheckView에서 연결이 끊겨BrowsingView로 돌아왔을 때 끊긴 기기가 연결되었다는 토스트가 나온다.ResultView에서 홈으로 돌아가는 알럿의 문구 수정 + 홈으로 돌아가는 액션 취소할 수 있도록 수정💬 리뷰 노트
browsingEventStream과 connectionCheckHeartbeatStream 수정
AsyncStream은 처음 구독한 곳에서만 사용됩니다. → 단일 구독자BrowsingView에서ConnectionCheckView로 갔다가 돌아오면.task가 다시 실행되지만,browsingEventStream은 초기화 시 한 번만 생성되므로 두 번째 구독에서 이벤트를 받지 못했습니다.제 상황에서는 돌아왔을 때 감지되는 새로운 기기가 UI에 반영되지 않던 상황이었습니다.
browsingEventStream을 computed property로 변경하여 접근할 때마다 새 스트림과 continuation을 생성하도록 수정했습니다.ConnectionCheckView도BrowsingView로 되돌아갔다가 다시 진입했을 때, 상대 기기의 연결 끊김을 감지하지 못했습니다.여기서도 같은 이유였고, 이에 따라 두 스트림의 형태를 변경했습니다.
다른 스트림은 생각했을 때 다시 재진입 할 화면이 보이지 않아 괜찮다고 판단했습니다.
기기 연결부터 사진 결과까지 전체 플로우를 2번 반복해도 문제가 없는 것도 확인했습니다.
토스트 코드 수정
문제 상황:
ConnectionCheckView전환BrowsingView로 돌아감원래 코드 흐름:
showToast = true)showToast = false로 바꾸는 타이머 시작onDisappear호출 → 타이머 취소됨showToast는true인 채로 남음BrowsingView로 돌아옴showToast가 아직true니까 → 토스트가 다시 뜸수정 후:
onDisappear에서 타이머 취소할 때showToast = false도 같이 해주면, 돌아와도 토스트가 안 뜸.