Standardized Drift errors and exceptions

[lenzpaul]

We are getting errors when our app tries to write to the database after the connection has been closed.

AFAIK, there is not a way to know whether the database connection is opened/closed, other than catching the error thrown by Drift when we try to access the closed database. This seems to be what was addressed in 2b96100480b3484139ba71538ccdc013e9193538, by throwing a StateError.

My issue is that StateError is a Dart generic error. This is not ideal for handling this exception, as it could come from anywhere in the code really.

Specifically, we are dealing with these 2 StateErrors - "Can't re-open a database after closing it. Please create a new database connection and open that instead." - "Tried to send ... over isolate channel, but the connection was closed!:

Would love to see an interface for all DriftException. This approach would provide us a clear and standardized way to handle and manage exceptions coming from your package.

Something like this for example

abstract class DriftError extends Error{}

class BadStateError extends DriftError{}

// etc

And thank you for an amazing package!

[simolus3]

Thanks for opening the issue and starting the discussion on this. I agree that a general type for drift exceptions is useful - it's a breaking change though.

When we throw a StateError (and other subclasses of Error) though, that generally indicates a API misuse and a bug in the way drift is used. I would prefer to keep throwing ArgumentErrors and StateErrors where applicable. You're not really supposed to be catching them - they shouldn't happen in the first place :) In this case, the question is why you're continuing to use the database after potentially closing it? Are there pending asynchronous tasks that might use the database outside of your control? If needed, there might be something missing in drift to better support closing the database when you're really done with it. We could add report information on whether the connection is open for instance. But it will generally be open until you close, so this should be under your control.

[lenzpaul]

@simolus3 Thank you for a prompt reply!

why you're continuing to use the database after potentially closing it? Are there pending asynchronous tasks that might use the database outside of your control?

Yes the challenge is that there are some pending async tasks that might use the database after it's been closed.

We could add report information on whether the connection is open for instance.

Yes, I this would be a great addition!

I agree that a general type for drift exceptions is useful - it's a breaking change though.

I understand that this would be breaking. How about using a mixin to inject the type in the interim? This would be non-breaking and allow for catching all errors thrown from Drift. Something like this:

/// Generic error from Drift.
///
/// Using mixin class to be able add this type to the inheritance chain and
/// still allow the subclass to extend other classes.
abstract mixin class DriftError {}

class DriftStateError extends StateError with DriftError {
  DriftStateError(String message) : super(message);
}

class DriftArgumentError extends ArgumentError with DriftError {
  DriftArgumentError([String? message]) : super(message);
}

void main() {
  try {
    throw DriftStateError('Drift State Error');
  } catch (e) {
    print("DriftStateError is StateError: ${e is StateError}"); // true
    print("DriftStateError is DriftError: ${e is DriftError}"); // true
  }

  try {
    throw DriftArgumentError('Drift Argument Error');
  } catch (e) {
    print("DriftArgumentError is ArgumentError: ${e is ArgumentError}"); // true
    print("DriftArgumentError is DriftError: ${e is DriftError}"); // true
  }

  // Would allow for this
  try {
    throw DriftStateError('Database is closed');
  } on DriftError catch (e) {
    print(e); // Bad state: Database is closed
  }
}

[lenzpaul]

@simolus3 Did you have a chance to look at my suggestion? What do you think of it?

[simolus3]

I'm still not convinced we should make subclasses of Error thrown by drift easier to catch. To quote the Error docs:

An Error object represents a program failure that the programmer should have avoided. These are not errors that a caller should expect or catch — if they occur, the program is erroneous, and terminating the program may be the safest response.

There might be places in drift where we're currently throwing an Error where we really should be throwing an exception subclass. I'll gladly revisit those occurrences if they seem overly strict in drift, but I agree with the notion that an error is a bug in the application. Making these easier to catch under an umbrella type goes against the idea of the Error type in my opinion.

That said, I still like the idea of a common DriftException type for things that aren't errors. We could possible have it as a mixin, so that things that are currently a SqliteException would be an internal DriftSqliteException type implementing both types, thus avoiding a breaking change.

Yes the challenge is that there are some pending async tasks that might use the database after it's been closed.

But why are you closing the database then? It's not necessarily a reliable detection, but we're using a finalizer to close the underlying sqlite3 database once there are no live references to it. So if you don't know when you're done using the database, perhaps not closing it is better than potentially throwing some tasks off (especially if they do non-transactional writes that might leave the database in an inconsistent state).

[lenzpaul]

I agree with the notion that an error is a bug in the application

I understand your point.

I still like the idea of a common DriftException type for things that aren't errors. We could possible have it as a mixin, so that things that are currently a SqliteException would be an internal DriftSqliteException type implementing both types, thus avoiding a breaking change.

Happy to help if I can

But why are you closing the database then?

There are futures that are queued before the database is potentially closed. We close the database on logout. In this case, we log the user out if they refuse to re-authenticate. It is a challenge to prevent or cancel these pending calls.

If we there was a way for us to know whether the database in a state where we can read/write from it that would solve it. I just really feel like there should be a isOpened property somewhere... @simolus3

[lenzpaul]

To be clear, once the database is closed we don't want to use it anymore. This is more of a side effect.

If we really can't check if the db is opened/closed, and also we can't catch the error thrown if the database is accessed after it's closed. We're kind of stuck, is there anything else that you can suggest?