Skip to content

Refactor shutdown procedure of sync isolates#421

Merged
simolus3 merged 4 commits into
mainfrom
sync-isolate-shutdown-refactor
May 27, 2026
Merged

Refactor shutdown procedure of sync isolates#421
simolus3 merged 4 commits into
mainfrom
sync-isolate-shutdown-refactor

Conversation

@simolus3
Copy link
Copy Markdown
Contributor

@simolus3 simolus3 commented May 26, 2026

Starting from version 2.0.0 of the SDK, the sync isolate opens a proper second reference to the database pool used by the main app. This includes spawning additional isolates to dispatch queries, and those isolates aren't properly closed because the sync isolate doesn't await database.close() before killing itself.

The fix for that alone would be simple, add an await and things should work. However, this seemed like a good opportunity to investigate why Isolate.current.kill() is even necessary. It's kind of a code smell since we're supposed to have cleared all resources keeping the isolate alive at that point. This PR fixes several issues and refactors how the sync isolate is managed.

  1. Instead of sending untyped lists with string commands between the sync isolate and the main app, this introduces SyncIsolateToClientMessageType and ClientToSyncIsolateMessageType enums documenting available messages.
  2. Direct SendPort.send invocations are replaced with an extension type exposing messages as well-typed methods.
  3. We no longer send crud updates from the main isolate to the sync isolate (the sqlite_async package takes care of that for us since the 2.0.0 update, we can just listen on onChange).
  4. Avoid the use of Future.delayed in the streaming sync implementation: It internally starts a Timer, which will keep the isolate alive until the timer expires. There's no way to "unregister" a listener on futures, so this can't be cancelled. This refactors _delayRetry to properly clean up the timer when the sync is aborted. This makes sync isolate exit much quicker.
  5. Add IsolateResultCollection as a wrapper around IsolateResults to be able to abort isolate results properly (this didn't turn out to be the issue, but is still an improvement: We shouldn't have to wait for outstanding crud uploads before shutting down the sync isolate).

AI use disclaimer: isolate_completer_test.dart was generated by Claude, but manually revised.

Closes #419.

@simolus3 simolus3 marked this pull request as ready for review May 26, 2026 14:55
@simolus3 simolus3 requested a review from stevensJourney May 26, 2026 14:55
Copy link
Copy Markdown
Contributor

@stevensJourney stevensJourney left a comment

Choose a reason for hiding this comment

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

This looks like a great update.

@simolus3 simolus3 merged commit bb6399f into main May 27, 2026
13 checks passed
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.

Bug: sync isolate shutdown doesn't await database.close() — leaks connection pool isolates on every disconnect cycle

2 participants