Skip to content

Announce engine closure on disposal and terminal reconnect failure - #1268

Draft
hiroshihorie wants to merge 2 commits into
mainfrom
hiroshi/close-engine-on-terminal-paths
Draft

hiroshihorie wants to merge 2 commits into
mainfrom
hiroshi/close-engine-on-terminal-paths

Conversation

@hiroshihorie

@hiroshihorie hiroshihorie commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Disposing a Room or giving up on reconnection can tear down the engine without emitting EngineClosingEvent. Subscribers waiting for closure never receive it. An unrecoverable reconnect also left isClosed false.

This change centralizes terminal closure in an idempotent internal Engine.markClosed(). It marks the engine closed synchronously and queues EngineClosingEvent while the event emitter is still alive.

Disposal sequence

Room.dispose() calls engine.markClosed() before super.dispose() and before any disposal callbacks. This placement matters because Room's cleanup itself calls engine.cleanUp(), before it eventually calls engine.dispose(). Announcing closure only in Engine's constructor-registered disposal callback is too late for that first transport teardown.

Engine.dispose() also calls markClosed() before super.dispose(). Disposal callbacks run in reverse registration order, so callbacks registered later can safely await engine work that is already being cancelled. Engine's constructor-registered callback retains its cleanup sequence: clean up transports, dispose the emitter, dispose signal listeners, and release reliable receive state.

Explicit disconnect, retry exhaustion, and unrecoverable reconnect failure use the same closure helper before cleanup. Recoverable retries and full-reconnect transport replacement do not close the engine. Buffer waits started after closure reject immediately, even if the retained channel reports a low buffer. Closing-event delivery remains asynchronous; disposal does not await completion of arbitrary event handlers.

Relationship to other fixes

This PR is based directly on main; it is not stacked on #1267 or #1264.

Validation

  • Ten lifecycle regression tests cover closure events, recoverable versus terminal reconnect failures, waits started after closure, duplicate closure, and disposal ordering. Three ordering tests fail at this PR's original head: closure during a paused first transport teardown, closure before a later Room disposal callback, and closure before a later Engine disposal callback. Another test verifies that the closing event is delivered with Engine marked disposed but its separate emitter still alive.
  • Full standalone unit suite: 504 passed, 1 skipped.
  • flutter analyze, dart analyze web/, repository format, import order, version consistency, and git diff --check pass.
  • Temporary combination with Fix waiting for the data channel send buffer to drain #1267: all 18 targeted tests pass, including probes for active waits on disposal, retry exhaustion, and unrecoverable reconnect failure; cancellation while the first room transport teardown is paused; and a later Engine disposal callback awaiting an existing blocked buffer wait. Temporary integration changes were removed afterward.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the terminal engine-closure changes.

No open issues 🎉

What changed in this PR

This PR centralizes terminal engine closure so subscribers receive EngineClosingEvent before disposal or terminal reconnect cleanup begins. It complements the separate fixes for already-blocked buffer waits and disconnect-during-reconnect races.

Changes:

  • Adds idempotent closure handling before Room and Engine disposal callbacks and after terminal reconnect failures.
  • Rejects buffer waits started after closure while leaving recoverable reconnects open.
  • Adds ten lifecycle regression tests and a patch changeset.
File Description
test/​core/​engine_closing_test.dart Tests closure events, disposal ordering, reconnect outcomes, and post-closure waits.
lib/​src/​core/​room.dart Marks the engine closed before Room disposal callbacks run.
lib/​src/​core/​engine.dart Centralizes closure signaling and guards buffer waits after closure.
.changes/​engine-terminal-closure Records the lifecycle and buffer-wait fixes for release.

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants