744294 - spin event loop at xpcom-shutdown-threads waiting for all pending asyncClose calls to complete

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: nsPlacesAutoComplete doesn't wait for asyncClose to finish

https://tbpl.mozilla.org/?tree=Try&rev=bbfcd8e3654a

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 613849 [details] [diff] [review]
nsPlacesAutoComplete doesn't wait for asyncClose to finish

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

Is this to support some sort of fatal check on shutdown? Cause we actually don't care if this dies before completing, it's not important for autocomplete.

Since we now have a list of all the connections in Storage (used to track memory so far), wouldn't be simpler to do this in Storage leveraging that list (i.e. on shutdown spin in one single point until all the connections in the list are properly closed) rather than in each single component?

Re: the patch, I think you also have to handle the other connection in urlInlineAutocomplete in the same file, and you should use Services.tm to get the thread-manager service (also valid for Bug 744080).

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Is this to support some sort of fatal check on shutdown?

Also. We care that the thread is gone.

> Cause we actually
> don't care if this dies before completing, it's not important for
> autocomplete.
> 
> Since we now have a list of all the connections in Storage (used to track
> memory so far), wouldn't be simpler to do this in Storage leveraging that
> list (i.e. on shutdown spin in one single point until all the connections in
> the list are properly closed) rather than in each single component?

Probably, I will give that a try.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Sorry for the delay, I was preempted to work on a leak.

I have an initial implementation (+ a bug fix for a bug it found) in https://tbpl.mozilla.org/?tree=Try&rev=1d455465352b. If it is green I will split it up in the morning.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: wait for all async connections

https://tbpl.mozilla.org/?tree=Try&rev=96a094b9a549

This patch makes us wait for all async connections. I have placed the loop in xpcom-shutdown-threads. If we don't care that a particular connection writes data to disk during shutdown, all that we care is that its thread goes away at this point.

If we care that a particular connection has its data written to disk, this will cause any pending statements to execute, and we should find out about it once write is being poisoned.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 618650 [details] [diff] [review]
wait for all async connections

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

So yeah, I think we can go this way with a couple adjustements.
Once you post an updated patch, may you please get feedback (or even review) from Asuth? I'd like him to take a look at this.

::: storage/src/mozStorageConnection.h
@@ +175,5 @@
>     */
>    int stepStatement(sqlite3_stmt* aStatement);
>  
> +  bool isAsync() {
> +    return mAsyncExecutionThread != NULL;

!!mAsyncExecutionThread

::: storage/src/mozStorageService.cpp
@@ +347,5 @@
>      NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
>      nsresult rv = os->AddObserver(mObserver, "xpcom-shutdown", false);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    rv = os->AddObserver(mObserver, "xpcom-shutdown-threads", false);
> +    NS_ENSURE_SUCCESS(rv, rv);

Was the plan to exit(0) at xpcom-shutdown? shutdown-threads comes later than that, though it's the perfect fit for this kind of thread work.

@@ +859,5 @@
> +  if (strcmp(aTopic, "xpcom-shutdown-threads") == 0) {
> +    nsCOMPtr<nsIObserverService> os =
> +      mozilla::services::GetObserverService();
> +    os->RemoveObserver(this, "xpcom-shutdown-threads");
> +    for (;;) {

What happens if an add-on creates a connection and doesn't close it? And I think there may be many in the wild. Wouldn't this hang indefinitely?

I think we should add an additional check, not just check mAsyncExecutionThread but also mAsyncExecutionThreadShuttingDown, basically that asyncClose has been invoked.
Then your isAsync would likely become a isWaitingForAsyncClose()

@@ +862,5 @@
> +    os->RemoveObserver(this, "xpcom-shutdown-threads");
> +    for (;;) {
> +      nsTArray<nsRefPtr<Connection> > connections;
> +      getConnections(connections);
> +      bool AnyOpen = false;

lowercase please

@@ +872,5 @@
> +        if (!conn->isAsync())
> +          continue;
> +
> +        bool isReady;
> +        (void)conn->GetConnectionReady(&isReady);

either handle errors or init isReady to false

@@ +877,5 @@
> +        if (isReady) {
> +          AnyOpen = true;
> +          break;
> +      }
> +      }

indentation on the first brace is wrong

@@ +882,5 @@
> +      if (!AnyOpen)
> +        break;
> +      nsCOMPtr<nsIThread> thread = do_GetCurrentThread();
> +      NS_ProcessNextEvent(thread);
> +    }

can make this a bit cleaner as:

do {
  ...
  if (anyOpen) {
    process_next
  }
} while (anyOpen);

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: https://tbpl.mozilla.org/?tree=Try&rev=6bcfabe74e70

We will eventually do an _exit(0) after profile-before-change in release builds. In debug builds we will poison write to make sure no one writes to disk after that point.

This patch makes sure that any thread we can asyncClose on is actually closed by xpcom-shutdown-threads. This has two implications
* We should not get threads causing problem after xpcom-shutdown-threads
* It will cause any pending statements to execute, which with write poisoning will find any statements that should have been executed earlier or canceled.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #7)
> We will eventually do an _exit(0) after profile-before-change in release
> builds.

profile-before-change is quite sooner than xpcom-shutdown-threads, so that may indeed skip wanted shutdown queries work. I thought the idea was to spin _before_ exit(0) to allow for finalization tasks to properly run (being able to mark some statements as cancelable, and skip those, clearly).
Btw, if I got your idea correct, this initial spinning is just to figure out problematic calls to be fixed later?  In such a case, fine.

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> profile-before-change is quite sooner than xpcom-shutdown-threads, so that
> may indeed skip wanted shutdown queries work. I thought the idea was to spin
> _before_ exit(0) to allow for finalization tasks to properly run (being able
> to mark some statements as cancelable, and skip those, clearly).
> Btw, if I got your idea correct, this initial spinning is just to figure out
> problematic calls to be fixed later?  In such a case, fine.

This spinning should be after the point we want to do _exit(0). There is no need to join threads in a release build. This spinning serves two purposes:

* Making sure the threads are gone.
* Spinning the loop and *in combination with write poisoning*, finding statement that should have been executed earlier or canceled.

Comment 10

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

(Moving into storage because the fix is entirely inside storage now.)

Comment 11

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

Comment on attachment 621974 [details] [diff] [review]
https://tbpl.mozilla.org/?tree=Try&rev=6bcfabe74e70

Looks good.  I see that this bug was explicitly mentioned on the snappy minutes, so I'm assuming (based on that and the discussion in the bug) this is all part of a plan Taras signed off on and he's not going to hunt me down :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 621974 [details] [diff] [review]
https://tbpl.mozilla.org/?tree=Try&rev=6bcfabe74e70

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

thansk Andrew, I was thinking to move  the bug once collected your feedback (since we could have decided to go back to a Places fix), you saved me an edit :)

::: storage/src/mozStorageConnection.cpp
@@ +816,5 @@
> +bool
> +Connection::isAsyncClosing() {
> +  MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
> +  bool isReady;
> +  (void)GetConnectionReady(&isReady);

as said, either error check or initialize isReady to a default value.

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9277703e34bd

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> > +  (void)GetConnectionReady(&isReady);
> 
> as said, either error check or initialize isReady to a default value.

Sorry, I missed this. Given that GetConnectionReady is

 *_ready = (mDBConn != nsnull);

is it possible to just make it return a bool? If not, would you be OK with a patch just using mDBConn?

Comment 16

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #14)
> Sorry, I missed this. Given that GetConnectionReady is
> 
>  *_ready = (mDBConn != nsnull);
> 
> is it possible to just make it return a bool?

nope, this is an idl implementation, so it has to return a nsresult

> If not, would you be OK with a patch just using mDBConn?

I think in future we may have to check more stuff in GetConnectionReady (for example it may return false as soon as we call asyncClose, cause it's not really ready at all), and likely we'll have to revise this code, so I'd prefer keeping it for now, to be sure we catch it in future.

In the end, from a realistic point of view, nothing bad may happen for now (as you said it's just checking mDBConn), from a technical point of view though, if you'd not know what's inside GetConnectionReady, you may be using an uninitialized variable. Imho you could just initialize isReady to false, or do nothing, it's not a blocking thing.

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Add c++ GetConnectionReady

I didn't realize idl interfaces were so bad :-(

The attached patch introduces a c++ GetConnctionReady, which is non virtual and inline. The idl one just forwards to it.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 622132 [details] [diff] [review]
Add c++ GetConnectionReady

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

::: storage/src/mozStorageConnection.h
@@ +175,4 @@
>     */
>    int stepStatement(sqlite3_stmt* aStatement);
>  
> +  bool GetConnectionReady() {

Per convention across the codebase, this should just be ConnectionReady()

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9277703e34bd

Comment 20

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e5e8a0a47dc5

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e5e8a0a47dc5