722243 - Mark sql queries as cancellable

Status: RESOLVED INACTIVE

(Toolkit :: Storage, defect)

Description

[(dormant account)]

Should be able to detect when a query is getting out of hand and cancel it. We need to be able to mark sql queries so we know which ones are safe to cancel. Should do that by marking sql queries
-- cancellable;
comment

Comment 1

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

Using the sqlite3_progress_handler mechanism, or just between calls to sqlite3_step?

Also, can you elaborate on out-of-hand / is there a specific query you are thinking about?  Too many rows, too long a runtime, too many I/O accesses?

Comment 2

[(dormant account)]

I haven't decided on the mechanism. The idea is to catch sql queries(both valid expensive ones on the main thread and ones due to lock contention).

Comment 3

[(dormant account)]

Our immediate need is to cancel async queries on shutdown. During shutdown when we call close, then sqlite/storage would go through the list of queries that are either queued or currently executing and cancel them. Async queries are already supposed to be checking for errors.

Comment 4

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

(In reply to Taras Glek (:taras) from comment #3)
> Our immediate need is to cancel async queries on shutdown.

This sounds like a good idea, thanks for the clarification.

> Async queries are already supposed to be checking for errors.

So is the plan to cancel all queries, or just those that opt-in?

I could see a very strong argument for saying: "mozStorage's default shutdown model is crash-only; when we get that xpcom shutdown notice we will immediately cancel all async database work without issuing a COMMIT or generating any notifications".  In the event there are databases that have data we really don't want to lose, they would be expected to be held to the higher standard of marking as much of their stuff cancelable as possible and working with the performance team, etc.

I would definitely advise against generating a new class of errors for things that are not opt-in.  I know in Thunderbird we only expect error messages for primary key violations and for latent database corruption that was not obvious at startup.

Comment 5

[(dormant account)]

plan is to cancel opt-in queries for now.

Comment 6

[Vladan Djeric (:vladan)]

Attachment: Interruptible SQL patch, with places JS queries marked interruptible

Comment 7

[Manoj]

Marco, any update on the review for the patch?

Comment 8

[Marco Bonardo [:mak]]

24hrs, sorry for delay.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 622982 [details] [diff] [review]
Interruptible SQL patch, with places JS queries marked interruptible

Review of attachment 622982 [details] [diff] [review]:
-----------------------------------------------------------------

So, sounds like this makes so that we have to parse each query before running it to check for interruptable, that sounds more expensive than just adding a boolean "interruptable" attribute to mozIStorageBaseStatement.

Most of this review is thoughts and feedback, not mandatory, we may discuss about it and figure a way out surely.
Though I think we should pay more attention to scope creep here, let's keep this as "don't slowdown shutdown" for now.

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +328,5 @@
> +    if (sql) {
> +      const char * comment = strstr(sql, "--");
> +      if (comment && strstr(comment, "interruptible")) {
> +        MutexAutoLock lockedScope(mMutex);
> +        if (mConnection->areInterruptibleCancelled()) {

if we do this on asyncClose, we may then check something like isConnectionClosing()

::: storage/src/mozStorageConnection.cpp
@@ +936,5 @@
>    return srv & 0xFF;
>  }
>  
> +void
> +Connection::interruptCurrentSql() {

interruptPendingStatement

please MOZ_ASSERT(NS_IsMainThread())

Looks like this method is unused... Maybe plans changed while working on it?

@@ +939,5 @@
> +void
> +Connection::interruptCurrentSql() {
> +  MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
> +  if (mIsInterruptible) {
> +    sqlite3_interrupt(mDBConn);

MOZ_ASSERT(mDBConn) please

@@ +944,5 @@
> +  }
> +}
> +
> +void
> +Connection::markCurrentSqlInterruptible(bool aInterruptible)

markPendingStatementAsInterruptible

This can only happen off main-thread, indeed I'd like a MOZ_ASSERT(!NS_IsMainThread())

@@ +947,5 @@
> +void
> +Connection::markCurrentSqlInterruptible(bool aInterruptible)
> +{
> +  // Locking happens in AsyncStatementExecution::executeAndProcessStatement
> +  mIsInterruptible = aInterruptible;

I wonder if generally would not be better to keep a pointer to the currently running async statement in the connection itself, and then use its GetInterruptible, though for now the simple bool may be fine, since we only need that, so let's avoid that complication.

@@ +951,5 @@
> +  mIsInterruptible = aInterruptible;
> +}
> +
> +void
> +Connection::cancelAllInterruptibleSql() {

I don't think we need to distinguish the case of cancelling a single statement or also any future one, our use-case is clear, we should stick to it: cancel any pending and don't allow any new.
cancelAnyInterruptibleStatement may be fine

@@ +960,5 @@
> +  }
> +}
> +
> +bool
> +Connection::areInterruptibleCancelled()

So, if we use asyncClose detection this would become useless (and actually I don't like this name, but whatever :) )

::: storage/src/mozStorageConnection.h
@@ +291,5 @@
>  
>    /**
> +   * Tracks if currently executing async SQL statement is interruptible.
> +   */
> +  bool mIsInterruptible;

mIsPendingStatementInterruptible

@@ +292,5 @@
>    /**
> +   * Tracks if currently executing async SQL statement is interruptible.
> +   */
> +  bool mIsInterruptible;
> +  bool mCancelAllInterruptible;

mAllowInterruptibleStatements

though as I said we'd be better tracking whether asyncClose has been invoked...

::: storage/src/mozStorageService.cpp
@@ +344,5 @@
>      // observer service can only be used on the main thread.
>      nsCOMPtr<nsIObserverService> os =
>        mozilla::services::GetObserverService();
>      NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
> +    nsresult rv = os->AddObserver(mObserver, "quit-application-granted", false);

making this depending on the app sounds like a bit over the top, this is a core service, should problably rather listen to profile-before-change.

Though, what about instead making this happen automatically when asyncClose() is invoked on the connection? Our use-case is that when the connection gets closed any pending or interruptible statements should just be canceled.

@@ +860,5 @@
> +    // Interrupt non-critical SQL so it doesn't delay shutdown
> +    nsTArray<nsRefPtr<Connection> > connections;
> +    getConnections(connections);
> +    for (PRUint32 i = 0; i < connections.Length(); ++i) {
> +      connections[i]->cancelAllInterruptibleSql();

you should first check ConnectionReady().
note that SQLite documentation states calling sqlite3_interrupt on a closed connection has undefined behavior, so better being safe.

Comment 10

[Vladan Djeric (:vladan)]

(In reply to Marco Bonardo [:mak] from comment #9)
> So, sounds like this makes so that we have to parse each query before
> running it to check for interruptable, that sounds more expensive than just
> adding a boolean "interruptable" attribute to mozIStorageBaseStatement.

I assumed parsing time wasn't a major performance concern because we're already doing an I/O operation + it's off the main thread and doesn't hold a lock, but I was on the fence about it. I've redone the patch with the boolean attribute instead, it required changes to code that creates async statements.

> > +void
> > +Connection::interruptCurrentSql() {
> 
> Looks like this method is unused... Maybe plans changed while working on it?

I added it for future use, but I can take it out until we actually need it.

> @@ +960,5 @@
> > +  }
> > +}
> > +
> > +bool
> > +Connection::areInterruptibleCancelled()
> 
> So, if we use asyncClose detection this would become useless (and actually I
> don't like this name, but whatever :) )

We decided during IRC convo that we want to call interrupt well before asyncClose to reduce likelihood of main thread blocking on the async thread's interruptible SQL.


> ::: storage/src/mozStorageService.cpp
> @@ +344,5 @@
> >      // observer service can only be used on the main thread.
> >      nsCOMPtr<nsIObserverService> os =
> >        mozilla::services::GetObserverService();
> >      NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
> > +    nsresult rv = os->AddObserver(mObserver, "quit-application-granted", false);
> 
> making this depending on the app sounds like a bit over the top, this is a
> core service, should problably rather listen to profile-before-change.
>
> Though, what about instead making this happen automatically when
> asyncClose() is invoked on the connection? Our use-case is that when the
> connection gets closed any pending or interruptible statements should just
> be canceled.

We want to cancel interruptible SQL as soon as we know we're shutting down. If we do it later, we run the risk of the main thread trying to do cleanup with SQL and getting blocked waiting for async SQL to finish.

Comment 11

[Vladan Djeric (:vladan)]

Attachment: Interruptible SQL patch v2, "interruptible" is now a flag and not an SQL comment

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 630347 [details] [diff] [review]
Interruptible SQL patch v2, "interruptible" is now a flag and not an SQL comment

Review of attachment 630347 [details] [diff] [review]:
-----------------------------------------------------------------

Do you have any idea on how to test this? Looks complicated, thinking if it's worth the time...
Btw, I don't like the interface breakage, see below

::: storage/public/mozIStorageConnection.idl
@@ +163,5 @@
>     */
> +  mozIStorageAsyncStatement createAsyncStatement(
> +    in AUTF8String aSQLStatement,
> +    [optional] in bool aInterruptible
> +  );

hm, unfortunately this is breaking any cpp consumers, that we care a bit more for Storage, since it's a most common API to use cpp side. Also it's a blind boolean argument, indeed you had to comment about it in nsPlacesAutocomplete, let's make it explicit. So I don't like it much, since there are alternatives who can avoid that problem, that is either:
1. createInterruptibleAsyncStatement
2. interruptible attribute on mozIStorageAsyncStatement

The latter would probably be preferrable from a clean API point of view, though if the complication is higher I'd accept the former.  This should also simplify the patch.

I'd also appreciate (not mandatory though) if you'd split this into an implementation patch, and a separate part changing/fixing the consumers. that's easier to check for eventual regressions too.

::: storage/src/mozStorageAsyncStatement.cpp
@@ +128,5 @@
>  
>  nsresult
>  AsyncStatement::initialize(Connection *aDBConnection,
> +                           const nsACString &aSQLStatement,
> +                           bool aInterruptible)

why not having a separate setter to invoke after initialize()?

::: storage/src/mozStorageConnection.cpp
@@ +911,5 @@
> +void
> +Connection::cancelAnyInterruptibleStatement()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mDBConn);

I'd rather check ConnectionReady() that is the same for now, but may change in future.

::: storage/src/mozStorageService.cpp
@@ +819,5 @@
>  
>  NS_IMETHODIMP
>  Service::Observe(nsISupports *, const char *aTopic, const PRUnichar *)
>  {
> +  if (strcmp(aTopic, "quit-application-granted") == 0) {

please remove the observer.

I wonder if we should rather use profile-before-change (or profile-change-teardown), mostly cause this is platform, it's not mandatory to have an app sending quit-application-granted (see xpcshell tests for example).

Comment 13

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

If you want to test interrupting a query in-flight, you could create and register a custom SQLite function that just blocks on a mutex you control.  So you start the function, wait for it to wedge on the mutex (or not wait, if you don't care if you cancel it ahead of time), trigger cancellation, then release the mutex and make sure it canceled.

Comment 14

[Vladan Djeric (:vladan)]

I tested the patch by modifying sqlite code to busy-wait on a specific SQL string

Comment 15

[Florian Bender]

Vladan, is there anything blocking you here?

Comment 16

[Vladan Djeric (:vladan)]

(In reply to Florian Bender from comment #15)
> Vladan, is there anything blocking you here?

If I remember correctly from a year ago, :mak wasn't enthusiastic about the interface breakage in the patch and we were talking about better ways to implement this. I've been wanting to revisit this bug for a while but haven't found time. The "shutdown by exit(0)" work was also put on hold.
Do you have a new use case for this functionality?

Comment 17

[Florian Bender]

Thanks for the info. 

(In reply to Vladan Djeric (:vladan) from comment #16)
> Do you have a new use case for this functionality?

No, I just came across a user comment and revisited the exit(0) bugs, and noticed the review-'ed patch which did not get an update within a year, so I was wondering if something is blocking you. ;) Any timeframe when "shutdown by exit(0) work" is revisited?

Comment 18

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.