233 lines
11 KiB
Text
233 lines
11 KiB
Text
<!-- task=kotlin_impl plan=0 tag=API -->
|
|
|
|
# Code Review Reference - API
|
|
|
|
## 개요
|
|
|
|
task=kotlin_impl, plan=0, tag=API
|
|
|
|
## 구현 항목별 완료 여부
|
|
|
|
| 항목 | 완료 여부 |
|
|
|------|---------|
|
|
| [API-1] Kotlin 프로젝트 구조 및 proto 바인딩 설정 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-2] Communicator 구현 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-3] HeartbeatTimer 구현 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-4] BaseClient 구현 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-5] TcpClient / TcpServer 구현 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-6] WsClient / WsServer 구현 | [x] 구현 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-7] Heartbeat 통합 테스트 | [x] 작성 완료 / JVM 부재로 Gradle 검증 차단 |
|
|
| [API-8] Go ↔ Kotlin 크로스테스트 | [x] 작성 완료 / JVM 부재로 Kotlin 실행 차단 |
|
|
|
|
## 계획 대비 변경 사항
|
|
|
|
- `go/crosstest/go_dart.go`와 `go/crosstest/go_kotlin.go`에 `//go:build ignore`를 추가했다. 두 파일 모두 단독 `go run ./crosstest/<file>.go` 오케스트레이터라서 같은 `package main` 안에 둘 경우 `go test ./...`에서 `main` 및 helper symbol이 충돌하기 때문이다. 명시 파일 `go run`은 유지된다.
|
|
- `tools/check_proto_sync.sh`가 Kotlin proto copy도 검사하도록 확장했다. Java/Kotlin 생성용 `option java_*`만 canonical diff에서 제외한다.
|
|
- `README.md`와 `PROTOCOL.md`의 Kotlin 상태는 `Available`이 아니라 `In progress`로 기록했다. 현재 컨테이너에 Java runtime이 없어 Kotlin 테스트와 크로스테스트를 실제 통과시키지 못했기 때문이다.
|
|
- Heartbeat 통합 테스트는 `runTest` 가상 시간이 아니라 `runBlocking` 실제 시간 기반으로 작성했다. `BaseClient`가 `Dispatchers.IO` scope를 소유하는 현재 구조에서는 가상 스케줄러만으로 transport 통합 흐름이 진행되지 않는다.
|
|
- `DialWss`는 API 표면을 추가했지만, 주어진 `SSLContext`에서 `X509TrustManager`를 안전하게 복원하는 경로는 아직 보수적으로 제한되어 있다. WSS 실검증은 후속 JVM 환경에서 보강 대상이다.
|
|
|
|
## 주요 설계 결정
|
|
|
|
- Kotlin proto는 schema package를 추가하지 않고 Java/Kotlin generation option만 둔다. `typeNameOf`는 `descriptorForType.fullName`을 사용하므로 현재 wire key는 Go/Dart와 같은 `TestData`, `HeartBeat`이다.
|
|
- `Communicator`는 `Channel<QueuedPacket>(64)` 기반 단일 write loop로 stream write interleaving을 방지한다. pending request는 `responseNonce`로 제거하며, type mismatch는 waiting caller에 오류로 전달한다.
|
|
- `addListener`와 `addRequestListener`는 `ReentrantReadWriteLock` write lock 안에서 상호 배타 조건을 검사한다.
|
|
- `BaseClient.close()`는 `AtomicBoolean.compareAndSet(false, true)`로 멱등성을 보장하고, communicator shutdown, heartbeat stop, transport close, disconnect listener notify 순서로 처리한다.
|
|
- TCP는 4-byte big-endian length prefix와 64 MiB max packet guard를 적용한다. WebSocket은 binary frame 하나에 `PacketBase` protobuf bytes를 싣는다.
|
|
- Go crosstest runner는 Go 서버/Kotlin 클라이언트, Kotlin runner는 Kotlin 서버/Go 클라이언트 방향을 각각 담당한다.
|
|
|
|
## 리뷰어를 위한 체크포인트
|
|
|
|
- Communicator의 `addListener` / `addRequestListener` 상호 배타 로직이 race-free한가?
|
|
- `close()` 멱등성: `AtomicBoolean.compareAndSet` 패턴이 모든 코드 경로에서 일관되게 적용됐는가?
|
|
- writeLoop가 `writeQueue.close()` 후 정상 종료되는가? (pending done 채널에 오류 전달 여부 확인)
|
|
- TCP readLoop에서 `length > MAX_PACKET_SIZE` 조건이 실제로 disconnect를 트리거하는가?
|
|
- WsClient OkHttp `WebSocketListener.onFailure` → `onDisconnected` 경로가 누락되지 않았는가?
|
|
- HeartbeatTimer가 `BaseClient.close()` 이후 callback을 발사하지 않는가? (scope 취소 순서 확인)
|
|
- 크로스테스트 포트(29290, 29292, 29390, 29392)가 기존 테스트 포트와 충돌하지 않는가?
|
|
- typeName이 `TestData` (단순 이름, no package prefix)로 Go / Dart 측과 일치하는가?
|
|
|
|
## 검증 결과
|
|
|
|
_구현 에이전트가 각 중간 검증 및 최종 검증 명령 실행 후 출력을 여기에 붙여 넣는다._
|
|
|
|
### API-1 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew compileKotlin
|
|
|
|
ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
|
|
|
|
Please set the JAVA_HOME variable in your environment to match the
|
|
location of your Java installation.
|
|
```
|
|
|
|
### API-2 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew test --tests "*.CommunicatorTest"
|
|
|
|
ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
|
|
|
|
Please set the JAVA_HOME variable in your environment to match the
|
|
location of your Java installation.
|
|
```
|
|
|
|
### API-3 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew test --tests "*.HeartbeatTimerTest"
|
|
|
|
Not rerun separately: same Gradle/JVM blocker as API-2.
|
|
```
|
|
|
|
### API-4 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew compileKotlin
|
|
|
|
Not rerun separately: same Gradle/JVM blocker as API-1.
|
|
```
|
|
|
|
### API-5 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew test --tests "*.TcpTest"
|
|
|
|
Not rerun separately: same Gradle/JVM blocker as API-2.
|
|
```
|
|
|
|
### API-6 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew test --tests "*.WsTest"
|
|
|
|
Not rerun separately: same Gradle/JVM blocker as API-2.
|
|
```
|
|
|
|
### API-7 중간 검증
|
|
```
|
|
$ cd kotlin && ./gradlew test --tests "*.HeartbeatTest"
|
|
|
|
Not rerun separately: same Gradle/JVM blocker as API-2.
|
|
```
|
|
|
|
### API-8 중간 검증
|
|
```
|
|
$ cd go && PATH=/config/go-sdk/go/bin:/config/go/bin:$PATH GOCACHE=/tmp/go-build GOMODCACHE=/tmp/go-mod go run ./crosstest/go_kotlin.go
|
|
INFO typeName go=TestData
|
|
|
|
ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
|
|
|
|
Please set the JAVA_HOME variable in your environment to match the
|
|
location of your Java installation.
|
|
|
|
FAIL crosstest error=kotlin-client tcp/send-push failed waitErr=exit status 1 failed=[] missing=[1 2]
|
|
exit status 1
|
|
```
|
|
|
|
### 최종 검증
|
|
```
|
|
$ cd go && PATH=/config/go-sdk/go/bin:/config/go/bin:$PATH GOCACHE=/tmp/go-build GOMODCACHE=/tmp/go-mod go test ./...
|
|
? toki-labs.com/toki_socket/go [no test files]
|
|
? toki-labs.com/toki_socket/go/crosstest/dart_go_client [no test files]
|
|
? toki-labs.com/toki_socket/go/crosstest/kotlin_go_client [no test files]
|
|
? toki-labs.com/toki_socket/go/examples/tcp_echo [no test files]
|
|
? toki-labs.com/toki_socket/go/examples/ws_echo [no test files]
|
|
? toki-labs.com/toki_socket/go/packets [no test files]
|
|
ok toki-labs.com/toki_socket/go/test (cached)
|
|
|
|
$ ./tools/check_proto_sync.sh
|
|
Proto schemas are in sync.
|
|
|
|
$ cd kotlin && ./gradlew test
|
|
|
|
ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
|
|
|
|
Please set the JAVA_HOME variable in your environment to match the
|
|
location of your Java installation.
|
|
```
|
|
|
|
---
|
|
|
|
## 코드리뷰 결과
|
|
|
|
### 종합 판정
|
|
|
|
**WARN**
|
|
|
|
---
|
|
|
|
### 차원별 평가
|
|
|
|
| 차원 | 판정 | 비고 |
|
|
|------|------|------|
|
|
| 정확성 (Correctness) | Pass | 프로토콜 wire format, nonce 단조 증가, close-once, addListener/addRequestListener 상호 배타, pending request 취소 모두 올바름 |
|
|
| 완성도 (Completeness) | Pass | 8개 항목 전부 구현됨. proto sync 검증, Go 테스트 회귀 없음 확인 |
|
|
| 테스트 커버리지 (Test coverage) | Warn | HeartbeatTest의 reset 테스트가 실제 메시지 수신 경로를 커버하지 않음 |
|
|
| API 계약 (API contract) | Warn | `crosstest/` 소스가 main sourceset에 포함되어 라이브러리 JAR에 crosstest 코드가 실려감 |
|
|
| 코드 품질 (Code quality) | Warn | `WsServer.stop()`의 `InterruptedException` 미처리, `kotlin_go.kt` 내 `runBlocking` in listener |
|
|
| 계획 대비 변경 (Plan deviation) | Pass | 모든 변경이 CODE_REVIEW.md에 기록되고 이유가 명확함 |
|
|
| 검증 신뢰도 (Verification trust) | Pass | JVM 환경 부재로 Kotlin 검증 불가, 가능한 Go 검증은 모두 실행됨. 제약이 명확히 기록됨 |
|
|
|
|
---
|
|
|
|
### 발견된 문제
|
|
|
|
**Suggested — `WsServer.stop()`: `super.stop(1000)` `InterruptedException` 미처리**
|
|
`kotlin/src/main/kotlin/com/tokilabs/toki_socket/WsServer.kt:36-41`
|
|
|
|
`WebSocketServer.stop(int)` 는 Java 메서드로 `InterruptedException` 을 선언한다. Kotlin은 checked exception을 컴파일 타임에 강제하지 않지만, `runBlocking` 컨텍스트에서 `InterruptedException` 이 던져지면 현재 coroutine이 취소 신호로 처리해 `stop()` 이후 로직이 실행되지 않는다.
|
|
|
|
```kotlin
|
|
// 현재
|
|
super.stop(1000)
|
|
|
|
// 수정
|
|
runCatching { super.stop(1000) }
|
|
```
|
|
|
|
---
|
|
|
|
**Suggested — `crosstest/` 코드가 main sourceset에 포함됨**
|
|
`kotlin/build.gradle.kts:14`
|
|
|
|
```kotlin
|
|
named("main") {
|
|
kotlin.srcDir("crosstest")
|
|
}
|
|
```
|
|
|
|
`kotlin_go.kt` 와 `go_kotlin_client/` 의 `Main.kt` 가 라이브러리 JAR에 함께 포함된다. crosstest 코드는 별도 sourceset(`crosstestMain`) 이나 `application` 전용 소스 경로로 분리해야 한다.
|
|
|
|
---
|
|
|
|
**Suggested — `kotlin_go.kt` listener callback 내 `runBlocking` 사용**
|
|
`kotlin/crosstest/kotlin_go.kt:56-63`, `104-111`
|
|
|
|
```kotlin
|
|
addListenerTyped<TestData>(client.communicator) { data ->
|
|
...
|
|
if (valid) {
|
|
runBlocking { client.send(...) } // ← Dispatchers.IO 스레드 블로킹
|
|
}
|
|
}
|
|
```
|
|
|
|
`onReceivedData` 는 `TcpClient.readLoop` 의 `Dispatchers.IO` coroutine 내에서 호출된다. listener callback에서 `runBlocking` 을 호출하면 IO 스레드를 점유한다. `scope.launch { client.send(...) }` 또는 listener를 `suspend` 람다로 받는 패턴으로 교체해야 한다.
|
|
|
|
현재 `addListenerTyped` 의 시그니처가 `fn: (T) -> Unit` 이므로 즉시 수정하려면 코드 내부에서 `scope.launch { }` 로 감싸거나, crosstest 코드 한정으로 별도 scope를 사용해야 한다.
|
|
|
|
---
|
|
|
|
**Suggested — `HeartbeatTest.testHeartbeatResetOnReceive()`: 수신 경로 미검증**
|
|
`kotlin/src/test/kotlin/com/tokilabs/toki_socket/HeartbeatTest.kt:69-81`
|
|
|
|
테스트 이름은 "receive 시 타이머 리셋"이지만 실제로는 `sendHeartBeat()` 를 직접 두 번 호출한다. 실제 경로인 `onReceivedData()` → `sendHeartBeat()` 를 통한 타이머 리셋이 검증되지 않는다. 테스트에서 `TcpClient` 또는 `HeartbeatClient` 의 `communicator.onReceivedData(...)` 를 호출하고 heartbeat가 지연되는지 확인해야 한다.
|
|
|
|
---
|
|
|
|
**Nit — `Communicator.parse()` 가 public으로 노출됨**
|
|
`kotlin/src/main/kotlin/com/tokilabs/toki_socket/Communicator.kt:266`
|
|
|
|
Go 구현에서는 `parse` 가 unexported이다. Kotlin에서도 `internal` 로 제한하는 것이 적절하다. 현재 inline helper (`addRequestListenerTyped`) 는 `queuePacket`, `nextNonce` 만 필요하고 `parse` 를 직접 호출하지 않는다.
|
|
|
|
---
|
|
|
|
### 다음 단계
|
|
|
|
WARN: Suggested 항목을 수정하는 후속 PLAN.md 가 작성된다. 수정 완료 후 재리뷰.
|