Conversation
GRJeon
added
☑️ test
YunDaeHyeon
left a comment
YunDaeHyeon
left a comment
There was a problem hiding this comment.
📄 PR Code Review Report
🔍 Summary
이번 Pull Request는 Multipeer Connectivity 세션 관리를 크게 개선하는 중요한 리팩터링을 포함하고 있습니다. 기존에 미러링 및 명령 전송을 위해 분리되었던 MCSession을 단일 세션으로 통합하고, MultipeerHeader를 사용하여 데이터 유형(스트리밍/명령)을 구분하는 방식으로 변경되었습니다. 이는 코드의 복잡성을 줄이고 리소스 효율성을 높이는 긍정적인 변화입니다. 전반적으로 변경 사항이 양측(브라우저 및 광고주)에서 일관성 있게 적용된 것으로 보이며, 테스트 코드도 새로운 프로토콜에 맞춰 업데이트되었습니다.
주요 변경사항은 잘 처리된 것으로 보이나, 몇 가지 세부적인 개선 사항이 식별되었습니다.
⚠️ Key Issues
- 알 수 없는 멀티피어 헤더 처리 미흡:
Browser및Advertiser의session(_:didReceive:fromPeer:)메서드에서 수신된 데이터의 첫 번째 바이트(MultipeerHeader)가command또는streaming이 아닌 경우 해당 데이터를 조용히 무시하고 있습니다. 이는 예기치 않은 데이터 형식이나 프로토콜 불일치가 발생했을 때 디버깅을 어렵게 만들 수 있습니다. 최소한 경고 로그를 남겨서 알 수 없는 데이터가 수신되었음을 명시적으로 알려주는 것이 좋습니다.
🛠 Improvement Suggestions
- 명령 데이터 암호화 변경에 대한 문서화: 기존
commandSession은encryptionPreference: .none으로 설정되어 있었으나, 단일 세션으로 통합되면서 모든 데이터(명령 포함)가encryptionPreference: .required로 암호화되게 변경되었습니다. 이는 보안 측면에서 긍정적이지만, 변경된 사항과 그 결정에 대한 간략한 주석이나 문서화가 있다면 향후 유지보수 시 더 명확할 것입니다. - 연결 초과 시간(
timeout) 상수로 관리:Browser에서invitePeer의timeout값이10초에서20초로 변경되었습니다. 이 값은 네트워크 환경에 따라 중요할 수 있으므로, 하드코딩된 리터럴 대신 적절한 상수(예:MultipeerConstants.invitationTimeout)로 정의하여 관리하면 가독성과 유지보수성이 향상될 것입니다. - 연결 관련 로그 추가:
Browser에서session(_:peer:didChange:)메서드에browsingManager.stopIfBrowsing()또는startIfBrowsing()로직이 추가되었습니다. 이 로직이 언제 실행되고 어떤 의도를 가지고 있는지 명시하는 로그를 추가하면 연결 상태 변화와 브라우징 동작 간의 관계를 이해하는 데 도움이 될 것입니다.Advertiser측의connectionManager.stopIfAdvertising()/startIfAdvertising()도 마찬가지입니다.
✅ Positive Observations
- 세션 통합 및 프로토콜 명확화: 스트리밍과 명령 채널을 단일
MCSession으로 통합하고MultipeerHeader를 도입하여 데이터 유형을 명확히 구분한 것은 Multipeer Connectivity의 모범 사례에 가까우며, 코드 베이스를 단순화하고 잠재적인 리소스 중복을 줄이는 훌륭한 리팩터링입니다. - 일관된 변경 적용:
Browser와Advertiser양쪽 컴포넌트 모두에서 세션 관리 로직, 데이터 전송 및 수신 처리, 테스트 코드까지 새로운 프로토콜에 맞춰 일관성 있게 업데이트되었습니다. 이는 복잡한 변경 사항에서 발생할 수 있는 휴먼 에러를 최소화하는 좋은 접근 방식입니다. - 연결 상태에 따른 브라우징/광고 최적화:
MCSessionState.connecting일 때 브라우징/광고를 중지하고, 연결 성공/실패 시 다시 시작하는 로직이 추가되었습니다. 이는 불필요한 네트워크 활동을 줄이고 기기 배터리 소모를 최적화하는 현명한 개선 사항입니다. - 테스트 코드의 견고함: 새로운
MultipeerHeader프로토콜에 맞춰AdvertiserTests및BrowserTests내의didReceive테스트 케이스들이 모두 업데이트되어 변경 사항에 대한 테스트 커버리지를 잘 유지하고 있습니다. 특히 데이터에 헤더 바이트를 추가한 후 전달하는 방식은 실제 동작을 정확히 반영합니다.
| var payload = Data([MultipeerHeader.streaming.rawValue]) | ||
| payload.append(data) | ||
|
|
||
| do { | ||
| try mirroringSession.send(data, toPeers: connectedPeers, with: .unreliable) | ||
| try mirroringSession.send(payload, toPeers: connectedPeers, with: .unreliable) |
There was a problem hiding this comment.
payload를 구성할 때 Data에 배열로 한 이유가 PR에 써진 것처럼 0x00, 0x01로 구분하기 위함인가요?
sangYuLv
left a comment
sangYuLv
left a comment
There was a problem hiding this comment.
미러링 세션 하나로 합친 것 좋네요!
아래처럼 테스트 결과가 나왔는데, 초대 받는 측은 와이파이에 꼭 연결 되어야하는 건가보다 싶네요 🧐
| 아이폰 | 아이패드 | 결과 |
|---|---|---|
| 와이파이 킴 (연결은 안 함) | 와이파이 킴 (연결은 안 함) | ❌ 연결 안 됨 |
| 핫스팟 킴 | 핫스팟에 연결 | ❌ 연결 안 되고, 아이폰에서 연결 시도 중 앱 튕김 |
| 와이파이 킴 (연결은 안 함) | 와이파이 연결 | ✅ 연결 됨 |
| 와이파이 연결 | 와이파이 킴 (연결은 안 함) | ❌ 연결 안 됨 |
| payload, | ||
| toPeers: connectedPeers, | ||
| with: .reliable | ||
| ) |
There was a problem hiding this comment.
세션.send()를 호출할 때, 한 줄로 처리하기도 하고, 이렇게 매개변수 별로 줄바꿈을 하기도 하네요!
만약 린트에 걸리지 않는다면, sendStreamData()에 있는 것처럼 한 줄로 처리하도록 통일하면 어떨까요?
There was a problem hiding this comment.
sendStreamData에서만 한 줄로 처리하는 것으로 확인되어 여기서 개행 처리 하는 방향으로 통일했습니다. 매개변수가 나름 의미를 갖는 것들이 들어가면 보기 편한 것이 좋다고 생각해요. 한 줄이 좋다고 생각하신다면 추가 의견 남겨주세요
| commandManager.execute(data: payload) | ||
| } else if firstByte == MultipeerHeader.streaming.rawValue { | ||
| logger.info("스트림 세션에서 데이터 수신: \(payload.count) bytes") | ||
| } |
There was a problem hiding this comment.
명령이 들어왔을 때도 아래 스트림처럼 로그를 찍어도 좋을 것 같아요!
아니면, 브라우저는 스트림 세션에서 데이터를 수신할 일이 없을 것 같은데 아래처럼 커멘드만 처리할까요?
스트림 세션에 대한 수신 로그를 찍어야했던 이유가 있다면 설명부탁드립니다!
if (session === mirroringSession || session === remoteSession)
&& firstByte == MultipeerHeader.command.rawValue {
commandManager.execute(data: payload)
}4 participants
🔗 연관된 이슈
📝 작업 내용
📌 요약
🔍 상세
왜 바뀌어야 했는가?
멀티피어는 블루투스와 P2P 와이파이를 사용할 수 있음에도 와이파이 공유기에 연결되지 않은 상태에서는 연결이 원활하지 않았습니다. 와이파이가 연결되지 않은 상태에서도 로컬 네트워크를 사용해 앱을 사용할 수 있도록 개선하고자 했습니다.
command session, streaming session 통합 과정
Common/MultipeerHeader에서 확인할 수 있듯, 전송하는 데이터 앞에 0x00(커맨드) 또는 0x01(스트리밍)의 헤더를 붙여 데이터를 전송하는 방식으로 구분한 데이터를 하나의 세션을 통해 보내도록 수정했습니다.
Invite 과정 진입 시 browser, advertiser 일시 정지
https://loud-peach-31c.notion.site/Wireshark-3104c58a09fa80bf826af45748474bc2?source=copy_link
위 링크는 Wireshark를 통해 와이파이 없이 연결을 시도할 때 오가는 패킷 관측을 포함한 연구 내용입니다. 주요 포인트는 다음과 같습니다.
위 연구에서 관측한 결과 중에서, 잦은 Retransmission이 관측되는 이유는 advertiser나 browser가 블루투스를 사용하고, Invite가 P2P 와이파이를 사용하며 네트워크 칩이라는 하드웨어를 나눠 사용, 네트워크 간섭 등이 발생하는 탓으로 예상했습니다.
따라서 browser는 connecting 상태가 되는 순간, advertiser는 invite를 받는 순간 검색을 종료하고, 실패나 성공으로 완료되었을 때 다시 작동하도록 수정하였습니다.
Invite 타임아웃 10초->20초
테스트 중 타임아웃을 늘렸을 때 실제 연결 시간이 20초까지 걸리지 않더라도 안정적으로 연결이 되는 모습을 관측하여 적용하였습니다.
💬 리뷰 노트