1556459 - Consider allowing read-write connections to be interrupted

Status: NEW

(Toolkit :: Storage, enhancement, P3)

Description

[Lina Butler [:lina]]

We currently restrict mozIStorageConnection::interrupt to read-only connections. Exposing this for all connections unconditionally is dangerous, as it allows for data loss...but, for cases like the Sync mirror, it's totally fine and even desirable. In that case, we don't want a long-running merge to block other operations or shutdown, and we can always retry merging on the next sync. The tab autocomplete connection seems like another use case, though I imagine it doesn't write nearly enough to need interrupting.

Could we add an interruptible attribute to mozIStorageConnection? (Or add it as an option to Open{Unshared}Database—though we'd need clones to be able to override it). It can default to on for read-only and off for read-write, but the latter could still opt in if it wanted.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

I think :mak's choice to limit this to async read-only connections in the implementing bug 1320301 was an outgrowth of Places' complicated usage patterns that made it hard to reason about what exactly would be interrupted and so scoping it to read-only avoided foot-guns. Discussion starts at https://bugzilla.mozilla.org/show_bug.cgi?id=1320301#c8 through c10 I think.

If we're talking about making this possible on async read-write connections, that seems fine as the results are nicely decoupled. As long as callers don't freak out when they see SQLITE_INTERRUPT result codes and delete the database, it should be fine.

If we're talking about making it possible for sync read-write connections to be interrupted from a different thread, I think QuotaManager actually wants that, but I think we'd want that to be opted-into on a per-statement and/or invocation basis. That's because interrupting the sync API is basically like random fault injection over a huge mass of code that has never been tested with random fault injection.

Comment 2

[Lina Butler [:lina]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

If we're talking about making this possible on async read-write connections, that seems fine as the results are nicely decoupled. As long as callers don't freak out when they see SQLITE_INTERRUPT result codes and delete the database, it should be fine.

Yep, I imagine this would only be for async read-write connections.

If we're talking about making it possible for sync read-write connections to be interrupted from a different thread, I think QuotaManager actually wants that, but I think we'd want that to be opted-into on a per-statement and/or invocation basis. That's because interrupting the sync API is basically like random fault injection over a huge mass of code that has never been tested with random fault injection.

That's why interrupting a read-write sync connection makes me nervous, too—what's going be interrupted when? Who knows!

Comment 3

[Marco Bonardo [:mak]]

My main concern is with connections that can be used on both threads, then it's really hard to understand what you are interrupting. Obviously it is still dangerous, if the consumer interrupt a write that should have gone through, so this remains both powerful and dangerous. Though, I'm sure we can express this properly in the API comment.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

If we're talking about making this possible on async read-write connections, that seems fine as the results are nicely decoupled. As long as callers don't freak out when they see SQLITE_INTERRUPT result codes and delete the database, it should be fine.

This is indeed a concern that should be well documented in the API, consumers like cookies like to just throw away the database on any error, and that's not ok. So the consumer when deciding to use this dangerous API should ensure (even with explicit unit tests and assertions) to properly handle that kind of error.

Comment 4

[Lina Butler [:lina]]

The other thing that seems a lil' sketchy to me is this: https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/storage/mozStorageAsyncStatementExecution.cpp#235-245

Before bug 1435446, it was possible to permanently wedge the async thread. I couldn't ever reproduce it in an isolated test case using the SQLite C API directly, or even in an xpcshell test—but it was easy to repro manually (bug 1435446, comment 3). I wonder if the long hangs we're seeing in bug 1561467, comment 0 and bug 1326309, comment 23 are also caused by us looping infinitely on SQLITE_BUSY.

It'd be useful if interrupting a connection would also break that loop, in addition to calling sqlite3_interrupt.