314 lines
9.8 KiB
Text
314 lines
9.8 KiB
Text
# Dart close() 버그 수정 및 Go 정렬
|
|
|
|
<!-- task=dart_close_fix plan=0 tag=REVIEW_DART_API -->
|
|
|
|
## 이 파일을 읽는 구현 에이전트에게
|
|
|
|
- 각 항목의 체크리스트를 순서대로 완료한다.
|
|
- 항목 완료마다 **중간 검증** 커맨드를 실행하고 통과를 확인한다.
|
|
- 모든 항목 완료 후 **최종 검증** 커맨드를 실행한다.
|
|
- 작업이 끝나면 `CODE_REVIEW.md`의 각 섹션을 실제 구현 내용으로 채운다.
|
|
|
|
## 배경
|
|
|
|
코드 리뷰에서 세 가지 이슈가 발견됐다.
|
|
가장 중요한 것은 `sendRequest` 호출 도중 `close()`가 불리면 반환된 `Future`가
|
|
영원히 완료되지 않는 버그다 (Go는 `closed` 채널로 즉시 해제함).
|
|
나머지 두 항목은 unawaited Future 경고와 Go/Dart 간 disconnect 통보 순서 비대칭 문제다.
|
|
|
|
---
|
|
|
|
### [REVIEW_DART_API-1] `sendRequest` pending future hang
|
|
|
|
**문제**
|
|
|
|
`dart/lib/src/communicator.dart:64-88` — `sendRequest`는 `_pendingRequests`에
|
|
`_PendingRequest`를 추가하고 `completer.future`를 반환한다.
|
|
`close()`가 호출돼도 `_pendingRequests`의 completer에 에러를 전달하는 코드가 없어
|
|
해당 future는 영원히 미완료 상태로 남는다.
|
|
|
|
Go 구현 (`go/communicator.go:196-210`)은 `SendRequest` 내부의 `select`에
|
|
`case <-c.closed:` 분기를 두어 연결 종료 즉시 `ErrNotConnected`를 반환한다.
|
|
|
|
**해결 방법**
|
|
|
|
`Communicator`에 `@protected cancelPendingRequests()` 메서드를 추가하고,
|
|
`BaseClient.close()` 내부에서 `isAlive = false` 직후에 호출한다.
|
|
|
|
```dart
|
|
// dart/lib/src/communicator.dart — cancelPendingRequests() 추가
|
|
// ignore_for_file 지시어 아래, Communicator 클래스 내부
|
|
|
|
@protected
|
|
void cancelPendingRequests() {
|
|
final snapshot = Map<int, _PendingRequest>.from(_pendingRequests);
|
|
_pendingRequests.clear();
|
|
for (final pending in snapshot.values) {
|
|
pending.completeError(StateError('connection closed'), StackTrace.current);
|
|
}
|
|
}
|
|
```
|
|
|
|
`_PendingRequest`에 `completeError` 메서드가 이미 있으므로 (`communicator.dart:198`)
|
|
추가 변경 없이 호출 가능하다.
|
|
|
|
```dart
|
|
// dart/lib/src/base_client.dart — close() 수정
|
|
// Before — base_client.dart:31-45
|
|
@override
|
|
Future<void> close() async {
|
|
if (!isAlive) return;
|
|
isAlive = false;
|
|
stopHeartbeat();
|
|
|
|
final listeners = List<void Function(Self)>.from(_disconnectListeners);
|
|
_disconnectListeners.clear();
|
|
for (final fn in listeners) fn(_self);
|
|
|
|
await closeTransport();
|
|
}
|
|
|
|
// After
|
|
@override
|
|
Future<void> close() async {
|
|
if (!isAlive) return;
|
|
isAlive = false;
|
|
stopHeartbeat();
|
|
cancelPendingRequests(); // ← 추가
|
|
|
|
final listeners = List<void Function(Self)>.from(_disconnectListeners);
|
|
_disconnectListeners.clear();
|
|
for (final fn in listeners) fn(_self);
|
|
|
|
await closeTransport();
|
|
}
|
|
```
|
|
|
|
**수정 파일 및 체크리스트**
|
|
|
|
- [ ] `dart/lib/src/communicator.dart`
|
|
- [ ] `import 'package:meta/meta.dart';` 가 이미 있는지 확인 (있음 — 22행)
|
|
- [ ] `cancelPendingRequests()` 메서드를 `Communicator` 클래스 안에 추가
|
|
- `@protected` 어노테이션 적용
|
|
- `_pendingRequests`를 snapshot → clear → 각 항목 `completeError(StateError('connection closed'), StackTrace.current)` 호출 순으로 구현
|
|
- [ ] `dart/lib/src/base_client.dart`
|
|
- [ ] `close()` 내부 `stopHeartbeat()` 호출 다음 줄에 `cancelPendingRequests();` 추가
|
|
|
|
**테스트 작성**
|
|
|
|
새 공개 API 동작이므로 회귀 테스트 필수.
|
|
|
|
| 파일 | `dart/test/communicator_test.dart` |
|
|
|------|--------------------------------------|
|
|
| 테스트 이름 | `'close 후 sendRequest는 StateError로 완료된다'` |
|
|
| 픽스처 | 기존 `_FakeCommunicator` 사용 (`isAlive = true`로 초기화됨) |
|
|
| 검증 목표 | `sendRequest` 호출 → `Future.delayed(Duration.zero)` → `isAlive = false` 직접 설정 + `cancelPendingRequests()` 호출 → future가 `StateError('connection closed')` 로 완료되는지 확인 |
|
|
|
|
```dart
|
|
test('close 후 sendRequest는 StateError로 완료된다', () async {
|
|
final communicator = _FakeCommunicator();
|
|
final future = communicator.sendRequest<TestData, TestData>(
|
|
TestData()..index = 1,
|
|
);
|
|
|
|
await Future<void>.delayed(Duration.zero);
|
|
communicator.isAlive = false;
|
|
communicator.cancelPendingRequests();
|
|
|
|
await expectLater(
|
|
future,
|
|
throwsA(
|
|
isA<StateError>().having(
|
|
(e) => e.message,
|
|
'message',
|
|
contains('connection closed'),
|
|
),
|
|
),
|
|
);
|
|
});
|
|
```
|
|
|
|
주의: `_FakeCommunicator`는 `Communicator`를 직접 상속하므로 `@protected` 메서드를
|
|
테스트 코드에서 직접 호출할 수 있다.
|
|
(`cancelPendingRequests()`는 `@protected`지만 같은 라이브러리 내이면 접근 가능.
|
|
테스트 파일은 별도 패키지이므로 `_FakeCommunicator`에 wrapper를 두거나
|
|
`communicator_test.dart`에서 subclass로 노출 필요 — 구현 에이전트가 판단.)
|
|
|
|
**중간 검증**
|
|
|
|
```bash
|
|
cd dart && dart analyze
|
|
cd dart && dart test test/communicator_test.dart
|
|
```
|
|
|
|
Expected: `No issues found!` + communicator 테스트 전량 통과
|
|
|
|
---
|
|
|
|
### [REVIEW_DART_API-2] `onDisconnectedClient` unawaited `close()`
|
|
|
|
**문제**
|
|
|
|
`dart/lib/src/protobuf_server.dart:66-69`와
|
|
`dart/lib/src/ws_protobuf_server.dart:66-69`에서
|
|
`void onDisconnectedClient(...)` 콜백이 `client.close()`를 await 없이 호출한다.
|
|
`stop()`은 `await client.close()`를 쓰는 반면 콜백은 그렇지 않아 일관성이 없고
|
|
dart analyzer의 `discarded_futures` 경고 대상이다.
|
|
|
|
**해결 방법**
|
|
|
|
`dart:async`의 `unawaited()`를 사용해 의도적으로 fire-and-forget임을 명시한다.
|
|
|
|
```dart
|
|
// Before — protobuf_server.dart:66-69
|
|
void onDisconnectedClient(ProtobufClient client) {
|
|
_clientList.remove(client);
|
|
client.close();
|
|
}
|
|
|
|
// After
|
|
void onDisconnectedClient(ProtobufClient client) {
|
|
_clientList.remove(client);
|
|
unawaited(client.close());
|
|
}
|
|
```
|
|
|
|
`ws_protobuf_server.dart`도 동일하게 적용한다.
|
|
|
|
**수정 파일 및 체크리스트**
|
|
|
|
- [ ] `dart/lib/src/protobuf_server.dart`
|
|
- [ ] `import 'dart:async';` 이미 없으면 추가 (파일 상단 확인 필요)
|
|
- [ ] `onDisconnectedClient` 내 `client.close()` → `unawaited(client.close())`
|
|
- [ ] `dart/lib/src/ws_protobuf_server.dart`
|
|
- [ ] `import 'dart:async';` 이미 없으면 추가
|
|
- [ ] `onDisconnectedClient` 내 `client.close()` → `unawaited(client.close())`
|
|
|
|
**테스트 작성**
|
|
|
|
내부 변경이므로 기존 테스트 통과로 충분. 신규 테스트 불필요.
|
|
|
|
**중간 검증**
|
|
|
|
```bash
|
|
cd dart && dart analyze
|
|
cd dart && dart test
|
|
```
|
|
|
|
Expected: `No issues found!` + 기존 39개 + 신규 1개(REVIEW_DART_API-1) = 40개 통과
|
|
|
|
---
|
|
|
|
### [REVIEW_DART_API-3] disconnect 통보 순서 — Go 기준으로 정렬
|
|
|
|
**문제**
|
|
|
|
Go `base_client.go:45-56`의 `Close()` 순서:
|
|
```
|
|
shutdown() → stopHeartbeat() → doClose() [transport] → notifyDisconnected() [listeners]
|
|
```
|
|
|
|
Dart `base_client.dart:31-45`의 현재 `close()` 순서:
|
|
```
|
|
isAlive=false → stopHeartbeat() → [listeners 통보] → closeTransport()
|
|
```
|
|
|
|
Dart는 listener가 호출되는 시점에 transport가 아직 열려 있다.
|
|
리스너 안에서 `send()`를 시도하면 `isAlive == false`에 의해 차단되므로
|
|
현재는 문제가 없으나, Go와 의미론이 달라 혼란을 유발한다.
|
|
Go의 의도: "transport가 완전히 닫힌 후에 외부에 알린다."
|
|
|
|
**해결 방법**
|
|
|
|
`BaseClient.close()`에서 `closeTransport()` 호출을 listener 통보보다 앞으로 이동한다.
|
|
|
|
```dart
|
|
// Before — base_client.dart:31-45
|
|
@override
|
|
Future<void> close() async {
|
|
if (!isAlive) return;
|
|
isAlive = false;
|
|
stopHeartbeat();
|
|
cancelPendingRequests();
|
|
|
|
final listeners = List<void Function(Self)>.from(_disconnectListeners);
|
|
_disconnectListeners.clear();
|
|
for (final fn in listeners) fn(_self);
|
|
|
|
await closeTransport();
|
|
}
|
|
|
|
// After
|
|
@override
|
|
Future<void> close() async {
|
|
if (!isAlive) return;
|
|
isAlive = false;
|
|
stopHeartbeat();
|
|
cancelPendingRequests();
|
|
|
|
await closeTransport(); // ← transport 먼저 닫고
|
|
|
|
final listeners = List<void Function(Self)>.from(_disconnectListeners);
|
|
_disconnectListeners.clear();
|
|
for (final fn in listeners) fn(_self);
|
|
}
|
|
```
|
|
|
|
기존 테스트에서 listener 통보와 transport 종료 사이의 순서를 직접 검증하는 케이스는 없으므로
|
|
순서 변경 후에도 기존 39개 + 신규 1개 = 40개 테스트가 그대로 통과해야 한다.
|
|
|
|
**수정 파일 및 체크리스트**
|
|
|
|
- [ ] `dart/lib/src/base_client.dart`
|
|
- [ ] `close()` 내부에서 `await closeTransport()` 를 listener 통보 블록보다 앞으로 이동
|
|
(REVIEW_DART_API-1의 `cancelPendingRequests()` 추가 후 그 바로 아래)
|
|
|
|
**테스트 작성**
|
|
|
|
순서 변경이므로 기존 테스트 통과로 충분. 신규 테스트 불필요.
|
|
|
|
**중간 검증**
|
|
|
|
```bash
|
|
cd dart && dart analyze
|
|
cd dart && dart test
|
|
```
|
|
|
|
Expected: `No issues found!` + 40개 전량 통과
|
|
|
|
---
|
|
|
|
## 의존 관계 및 구현 순서
|
|
|
|
```
|
|
[REVIEW_DART_API-1] → [REVIEW_DART_API-2] → [REVIEW_DART_API-3]
|
|
```
|
|
|
|
- 1번이 `cancelPendingRequests()`를 `Communicator`와 `BaseClient.close()`에 추가한다.
|
|
- 2번과 3번은 독립적이나 1번 완료 후 `dart analyze`를 확인하고 진행한다.
|
|
- 3번은 `BaseClient.close()`를 한 번만 수정하므로 1번 diff와 합산 적용한다.
|
|
|
|
---
|
|
|
|
## 수정 파일 요약
|
|
|
|
| 파일 | 항목 |
|
|
|------|------|
|
|
| `dart/lib/src/communicator.dart` | REVIEW_DART_API-1 |
|
|
| `dart/lib/src/base_client.dart` | REVIEW_DART_API-1, REVIEW_DART_API-3 |
|
|
| `dart/lib/src/protobuf_server.dart` | REVIEW_DART_API-2 |
|
|
| `dart/lib/src/ws_protobuf_server.dart` | REVIEW_DART_API-2 |
|
|
| `dart/test/communicator_test.dart` | REVIEW_DART_API-1 (새 테스트) |
|
|
|
|
---
|
|
|
|
## 최종 검증
|
|
|
|
```bash
|
|
# 1. 정적 분석
|
|
cd dart && dart analyze
|
|
# 2. 전체 테스트
|
|
cd dart && dart test
|
|
```
|
|
|
|
Expected outcome: `No issues found!` + 기존 39개 + 신규 1개 = **총 40개 통과**
|