fix: Ensure close is idempotent #295
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR refactors isolate synchronization syntax from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–60 minutes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
=======================================
Coverage 91.99% 92.00%
=======================================
Files 97 97
Lines 3662 3664 +2
Branches 1881 1881
=======================================
+ Hits 3369 3371 +2
Misses 293 293 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/relic_server.dart (1)
256-258: Consider consistent error handling forportafter close.After
close()clears_children, callingportthrowsStateError: No elementfromList.first. In contrast,_RelicServer.port(line 83) throwsStateError: Not bound.For consistency, you could cache the port similarly to
_RelicServer:+ int? _port; @override - int get port => _children.first.port; + int get port => _port ?? (throw StateError('Not bound'));And update
close()to also clear_port:@override Future<void> close() async { final children = List.of(_children); _children.clear(); + _port = null; await children.map((final c) => c.close()).wait; }And set
_portinmountAndStart:@override Future<void> mountAndStart(final Handler handler) async { await _children.map((final c) => c.mountAndStart(handler)).wait; + _port ??= _children.first.port; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
example/advanced/multi_isolate.dart(1 hunks)lib/src/relic_server.dart(1 hunks)test/isolated_object/isolated_object_evaluate_test.dart(2 hunks)test/relic_server_graceful_shutdown_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.dart: All Dart code must pass static analysis usingdart analyze --fatal-infoswith no issues
All Dart files must be formatted withdart format(CI enforcesdart format --set-exit-if-changed .)
Files:
test/isolated_object/isolated_object_evaluate_test.darttest/relic_server_graceful_shutdown_test.dartlib/src/relic_server.dartexample/advanced/multi_isolate.dart
test/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.dart: Tests should follow the Given-When-Then pattern in descriptions (flexible structuring allowed)
Use Arrange-Act-Assert pattern within test bodies
Provide clear, descriptive test titles; prefer single responsibility per test unless related assertions improve clarity
Place tests in thetest/directory mirroring thelib/structure
Files:
test/isolated_object/isolated_object_evaluate_test.darttest/relic_server_graceful_shutdown_test.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
lib/**/*.dart: Use Uint8List for request/response bodies for performance; avoid List for body payloads
Use type-safe HTTP header parsing and validation when accessing headers
Use router with trie-based matching and symbol-based path parameters (e.g.,#name,#age) for routing
Ensure WebSocket handling includes proper lifecycle management (e.g., ping/pong for connection health)
Files:
lib/src/relic_server.dart
🧠 Learnings (8)
📓 Common learnings
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: example/example.dart:31-36
Timestamp: 2025-04-24T14:06:32.810Z
Learning: In the example code, `sleep()` is intentionally used instead of `await Future.delayed()` to simulate CPU-bound work that benefits from multiple isolates/cores. Using a blocking call demonstrates why multiple isolates are necessary, while an async approach would allow a single isolate to handle multiple requests concurrently, defeating the purpose of the multi-isolate example.
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: lib/src/handler/handler.dart:59-67
Timestamp: 2025-04-25T07:39:38.915Z
Learning: Nielsenko prefers using switch statements with pattern matching over if statements when working with sealed classes in Dart, as they provide exhaustiveness checking at compile time and can be more concise.
📚 Learning: 2025-04-24T14:06:32.810Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: example/example.dart:31-36
Timestamp: 2025-04-24T14:06:32.810Z
Learning: In the example code, `sleep()` is intentionally used instead of `await Future.delayed()` to simulate CPU-bound work that benefits from multiple isolates/cores. Using a blocking call demonstrates why multiple isolates are necessary, while an async approach would allow a single isolate to handle multiple requests concurrently, defeating the purpose of the multi-isolate example.
Applied to files:
test/isolated_object/isolated_object_evaluate_test.darttest/relic_server_graceful_shutdown_test.dartexample/advanced/multi_isolate.dart
📚 Learning: 2025-10-09T16:21:09.310Z
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to test/**/*.dart : Provide clear, descriptive test titles; prefer single responsibility per test unless related assertions improve clarity
Applied to files:
test/relic_server_graceful_shutdown_test.dart
📚 Learning: 2025-10-09T16:21:09.310Z
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to lib/**/*.dart : Ensure WebSocket handling includes proper lifecycle management (e.g., ping/pong for connection health)
Applied to files:
test/relic_server_graceful_shutdown_test.dart
📚 Learning: 2025-04-24T04:14:12.943Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 47
File: test/hijack/relic_hijack_test.dart:82-90
Timestamp: 2025-04-24T04:14:12.943Z
Learning: Tests within a single file in Dart's test package run sequentially, not concurrently, so global state for test resources within a file doesn't present race condition risks.
Applied to files:
test/relic_server_graceful_shutdown_test.dart
📚 Learning: 2025-10-22T11:25:39.264Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 216
File: lib/src/router/relic_app.dart:47-49
Timestamp: 2025-10-22T11:25:39.264Z
Learning: In the serverpod/relic repository, validation of the `noOfIsolates` parameter should be handled in the `RelicServer` constructor (lib/src/relic_server.dart), not in `RelicApp.run` (lib/src/router/relic_app.dart).
Applied to files:
test/relic_server_graceful_shutdown_test.dart
📚 Learning: 2025-05-22T15:55:46.307Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 79
File: benchmark/benchmark.dart:0-0
Timestamp: 2025-05-22T15:55:46.307Z
Learning: When working with Dart's IOSink (e.g., from File.openWrite()), always ensure to properly close it when done to flush any buffered data and release system resources. Without explicit closing, the last buffer may not be flushed, potentially resulting in data loss.
Applied to files:
lib/src/relic_server.dart
📚 Learning: 2025-10-22T11:21:50.149Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 216
File: lib/src/isolated_object.dart:6-7
Timestamp: 2025-10-22T11:21:50.149Z
Learning: In Dart, closures CAN be sent between isolates when the isolates share the same code (e.g., using Isolate.spawn or Isolate.run). The closure and any captured state must be sendable. This is officially supported and documented in Dart's isolate documentation. Passing function objects like `dynamic Function(T)` between isolates is a valid pattern when the captured state is sendable.
Applied to files:
example/advanced/multi_isolate.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Run Unit Tests (windows-latest, 3.7.0, downgrade)
- GitHub Check: Run Unit Tests (windows-latest, stable, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, beta, upgrade)
- GitHub Check: Run Unit Tests (macos-latest, stable, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, beta, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, 3.7.0, downgrade)
- GitHub Check: Run Unit Tests (windows-latest, beta, downgrade)
- GitHub Check: Run Unit Tests (macos-latest, stable, upgrade)
- GitHub Check: Run Unit Tests (macos-latest, 3.7.0, upgrade)
- GitHub Check: Run Unit Tests (ubuntu-latest, stable, upgrade)
- GitHub Check: Run Unit Tests (windows-latest, 3.7.0, upgrade)
- GitHub Check: Run Unit Tests (ubuntu-latest, stable, downgrade)
- GitHub Check: Run Unit Tests (windows-latest, beta, upgrade)
- GitHub Check: Run Unit Tests (windows-latest, stable, upgrade)
- GitHub Check: Verify Build (ubuntu-latest, stable)
🔇 Additional comments (10)
test/isolated_object/isolated_object_evaluate_test.dart (2)
57-57: LGTM!Using
futures.wait(the Dart 3.0+FutureExtensionsonList<Future<T>>) is a cleaner, more idiomatic approach thanFuture.wait(futures). The behavior is equivalent here since no errors are expected.
193-193: LGTM!Consistent use of the
.waitextension pattern. The test correctly validates that all 100 operations complete successfully.lib/src/relic_server.dart (1)
230-234: Excellent fix for the idempotent close behavior.The copy-and-clear pattern correctly ensures that:
- Concurrent calls to
close()will see an empty_childrenlist after the first caller clears it- The first caller completes closing all children from its snapshot
- Subsequent calls await an empty list, completing immediately as no-ops
This synchronous copy-and-clear before any async operations effectively serializes the close semantics.
example/advanced/multi_isolate.dart (1)
15-20: LGTM!The refactor to use
List.generate(...).waitis cleaner and consistent with the.waitextension pattern used throughout this PR. The isolate spawning logic and debug naming remain unchanged.test/relic_server_graceful_shutdown_test.dart (6)
14-55: Well-designed test helpers for controlled shutdown testing.The separation between
_createSignalingHandler(for single-isolate tests with precise synchronization) and_createDelayedHandler(for multi-isolate tests where Completers can't cross boundaries) is a good design choice.Note: The 50ms delay in
_startDelayedInFlightRequests(line 52) is timing-dependent. If tests become flaky in slow CI environments, consider increasing this value. The 300ms default request delay provides reasonable margin.
98-144: Comprehensive test for graceful shutdown with in-flight requests.The test correctly:
- Starts requests and waits for them to begin processing
- Initiates server close while requests are in-flight
- Allows requests to complete
- Verifies all responses are successful
Good use of destructuring with
(:responseFutures, :canComplete)pattern.
146-197: Good handling of timing-dependent behavior.The flexible assertion at line 193 appropriately handles the race between the new request attempt and socket closure. The comment at lines 189-192 clearly explains why exact behavior varies.
199-231: Direct test coverage for issue #293.Excellent test cases that validate the core fix:
- Sequential double-close: Uses
expectLater(server.close(), completes)to detect hangs- Concurrent double-close: Tests the race condition scenario with in-flight requests
The issue references in comments provide good traceability.
234-252: Defensive tearDown handling for multi-isolate tests.The
serverClosedflag and try-catch intearDownis a good defensive pattern. Interestingly, once this PR's fix is merged, the double-close protection would make this simpler - but keeping it doesn't hurt and documents the expected behavior.Tests appropriately use
noOfIsolates: 2to validate multi-isolate shutdown.
254-303: Good test coverage for multi-isolate shutdown scenarios.The multi-isolate tests appropriately:
- Mirror the single-isolate test structure for consistency
- Use the delay-based helper since Completers can't cross isolate boundaries
- Cover both sequential and concurrent double-close scenarios
This ensures the snapshot-and-clear fix in
_MultiIsolateRelicServer.close()works correctly.
nielsenko
force-pushed
the
fix-double-close
branch
from
609c8d5 to
15e6f97
Compare
|
Rebased on main after merge of #292 |
2 participants
Description
close()is called multiple times_childrenlist before closing to make subsequent calls no-opsRelated Issues
Pre-Launch Checklist
///), ensuring consistency with existing project documentation.Breaking Changes
Additional Notes
Investigation confirmed that
HttpServer.close()andServerSocket.close()are idempotent in dart:io, making the fix safe. The multi-isolate fix uses a synchronous copy-and-clear pattern that acts as a simple mutex for concurrent calls.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.