Stronger Shutdown Semantics in Database

[hiranya911]

There are several limitations in the existing shutdown/destroy logic of FirebaseDatabase:

1. ctx.stop() is called conditionally, and this doesn't always seem to happen.
2. Some low-level websocket events like onClosed() fire late, usually after RunLoop.shutdown() has been called. In this case, the RunLoop is silently re-initialized to handle the late event.
3. Some tasks like idle time checker are not cleaned up.

Users have observed dangling background threads left by the SDK due to the above issues.

To resolve these problems, this PR:

1. Implements a new, stronger RepoManager.destroy() method that always calls ctx.stop().
2. Additional check for handling late onClosed() events, thus preventing the RunLoop from re-initializing after shutdown.
3. Cancels pending idle time check tasks on shutdown.
4. Enables removeOnCancelPolicy() on the RevivingScheduledExecutor, which removes cancelled tasks from the job queue thus facilitating early termination of threads (the underlying ScheduledThreadPoolExecutor won't shutdown threads, as long as there are jobs in the queue, even if they are cancelled).

I have tested this PR locally, and verified that all database-related threads are shut down upon calling FirebaseApp.delete().

[mikelehen]

Looks good, but a couple suggestions.

[mikelehen]

This seems to be a subset of interruptInternal() [it interrupts all the repos but never calls ctx.stop()]. Could we just use interruptInternal()?

[hiranya911]

We were effectively calling interruptInternal() before this PR. But it only calls ctx.stop() conditionally. Specifically, it won't close the context when there are active event listeners. The new destroy() method has stronger semantics in the sense it will always stop the context.

[mikelehen]

Yeah. My point was that destroy() could call interrupt() and then call ctx.stop() instead of duplicating this code. I don't care that strongly though.

[mikelehen]

I think you should probably duplicate the isClosed check here, since we're now on the correct thread (the one that mutates isClosed).

[hiranya911]

onClosed() method already has the check:

private void onClosed() {
    if (!isClosed) {
      if (logger.logsDebug()) {
        logger.debug("closing itself");
      }
      shutdown();
    }
    conn = null;
    if (keepAlive != null) {
      keepAlive.cancel(false);
      keepAlive = null;
    }
  }

[mikelehen]

should you set them to null too?

[hiranya911]

I think we need them to remain non-null for Context.restartServices() to work. In case of a destroy() it won't really matter either way.