Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disconnect()/connect() and status property values #252

Open
nurturenature opened this issue Mar 11, 2025 · 2 comments
Open

disconnect()/connect() and status property values #252

nurturenature opened this issue Mar 11, 2025 · 2 comments

Comments

@nurturenature
Copy link

Hi,

This is a breakout, decomposition from issue #226.

When disconnecting and connecting, there appear to be issues with status property values:

  • status property values

    • db.connected is not true after await'ing db.connect()
    • SyncStatus.uploading can be true even though SyncStatus.connected is false
  • db.connect() is not idempotent?

    • appears to trigger a disconnect
    • db.connected is not true after await'ing db.connect()

Although there can be database consistency anomalies triggered by disconnect()/connect(), this issue is only for the status property values. Maybe resolving these will expose other possible client library behavior? (A separate consistency issue will be opened.)

The status mismatches can be observed with any simple disconnect()/connect() behavior.

The test app used code like:

# test connecting
await _db.connect(connector: _connector);
final closed = _db.closed;
final connected = _db.connected;
final currentStatus = _db.currentStatus;
final v = {
  'db.closed': closed,
  'db.connected': connected,
  'db.currentStatus': currentStatus,
};
if (!connected) {
  log.warning(
    'PowerSyncDatabase: expected db.connected to be true after db.connect(): $v',
  );
}

# test disconnecting
await _db.disconnect();
final closed = _db.closed;
final connected = _db.connected;
final currentStatus = _db.currentStatus;
final v = {
  'db.closed': closed,
  'db.connected': connected,
  'db.currentStatus': currentStatus,
};
if (connected) {
  log.warning(
    'PowerSyncDatabase: expected db.connected to be false after db.disconnect(): $v',
  );
}

# listen for SyncStatus mismatches
db.statusStream.listen((syncStatus) {
  // state mismatch
  if (!syncStatus.connected &&
      (syncStatus.downloading || syncStatus.uploading)) {
    log.warning(
      'SyncStatus: syncStatus.connected is false yet uploading|downloading: $syncStatus',
    );
  }
  if ((syncStatus.hasSynced == false && syncStatus.lastSyncedAt != null) ||
      (syncStatus.hasSynced == true && syncStatus.lastSyncedAt == null)) {
    log.warning(
      'SyncStatus: syncStatus.hasSynced/lastSyncedAt mismatch: $syncStatus',
    );
  }
});

Behavior when going from connected state and calling disconnect(), and then going from disconnected state and calling connect().

# in connected state
# call disconnect()
# db and SyncStatus values correct
[2025-03-07 03:27:07.127957] [ps-7] [FINEST] SyncStatus<connected: false connecting: false downloading: false uploading: false lastSyncedAt: 2025-03-07 03:27:06.962466, hasSynced: true, error: null>
...
# in disconnected state
# call connect()
# invalid db.connected: false even after await'ing db.connect()
[2025-03-07 03:27:12.691361] [ps-7] [WARNING] PowerSyncDatabase: expected db.connected to be true after db.connect(): {db.closed: false, db.connected: false, db.currentStatus: SyncStatus<connected: false connecting: false downloading: false uploading: false lastSyncedAt: 2025-03-07 03:27:06.962466, hasSynced: true, error: null>}
...
# invalid uploading: true even when connected: false
[2025-03-07 03:27:12.706011] [ps-7] [WARNING] SyncStatus: syncStatus.connected is false yet uploading|downloading: SyncStatus<connected: false connecting: true downloading: false uploading: true lastSyncedAt: 2025-03-07 03:27:06.962466, hasSynced: true, error: null>
...
# SyncStatus catches up with correct connected state
[2025-03-07 03:27:12.746889] [ps-7] [FINEST] SyncStatus<connected: true connecting: false downloading: false uploading: true lastSyncedAt: 2025-03-07 03:27:06.962466, hasSynced: true, error: null>

Behavior when calling db.connect() when connected.

# connected state
# call connect()
...
# appears to disconnect?
# improper db.connected: false after await'ing a db.connect()
[2025-03-07 03:27:12.693706] [ps-3] [WARNING] PowerSyncDatabase: expected db.connected to be true after db.connect(): {db.closed: false, db.connected: false, db.currentStatus: SyncStatus<connected: false connecting: false downloading: false uploading: false lastSyncedAt: 2025-03-07 03:27:12.683099, hasSynced: true, error: null>}
...
# appears to (re)connect?
# SyncStatus moves through connecting: true to connected: true
[2025-03-07 03:27:12.708370] [ps-3] [FINEST] SyncStatus<connected: false connecting: true downloading: false uploading: false lastSyncedAt: 2025-03-07 03:27:12.683099, hasSynced: true, error: null>
[2025-03-07 03:27:12.739435] [ps-3] [FINEST] SyncStatus<connected: true connecting: false downloading: false uploading: false lastSyncedAt: 2025-03-07 03:27:12.683099, hasSynced: true, error: null>
@rkistner
Copy link
Contributor

To some extent this is expected with the current API design:

  1. Calling connect() is effectively an instruction to make a best-effort attempt to maintain a connection.
  2. SyncStatus reports the status of the actual connection, not of whether or not connect() was called.
  3. The Future returned from connect() waits for some intialization to complete, but does not wait for the actual connection, which could be much later.
  4. Since a different connector can be supplied on any call to connect(), we disconnect and re-connect with the new connector.
  5. Uploading, while typically only performed while being connected, is a separate process that can continue while the download connection is interrupted.

I do agree that this is not an ideal API design. Some simple changes I can think of:

  1. Expose an additional field on SyncStatus that indicates whether or not the database is in a "connect()" state, regardless of whether or not an actual connection is open (i.e. connect() has been called, disconnect() has not been called after that, and the database will attempt to maintain a connection).
  2. connect() can be idempotent if the same connector and parameters was passed to it again.

Would these changes help?

@nurturenature
Copy link
Author

Thanks for the explanations.

I think that SyncStatus reflecting the actual state, e.g.

... reports the status of the actual connection
... Uploading, while typically only performed while being connected, is a separate process ...
... etc

makes sense with the explanations.
Maybe some doc clarifications to avoid any assumptions around the semantics would be helpful.

I don't think

Expose an additional field on SyncStatus that indicates whether or not the database is in a "connect()" state...

is necessary or would be clarifying to the developer or the reactive UI.

Having connect() be idempotent with the same connector/parameters would be a good change to make:

  • disconnect() already is
  • possible to see how developers could be calling it that way, a UI being clicked expecting it that way
  • avoids possible unnecessary disconnect/connect churn

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

No branches or pull requests

2 participants