feat: add shutdown() and shutdownNow()

[schmidt-sebastian]

This adds a shutdown and a shutdownNow message. Unfortunately, it probably requires a major version bump since the method is added to the main interface (did we make exceptions before?).

[schmidt-sebastian]

@BenWhitehead This might actually be enough to solve the underlying issue in close(), which means that we might not need to expose a separate shutdown API. With the original code, the newly created firestoreStub does not reference the existing background resources of the clientContext and thus the shutdown call does not actually do anything.

I also do not know what the consequence of not providing the retry settings builder is.

[schmidt-sebastian]

I was able to add this back by adding the existing logic from close that manually closed the background resources.

[BenWhitehead]

I write the following Integration Test to test whether close hangs or not

`FirestoreCloseHangTest.java`

public final class FirestoreCloseHang {

  @Rule
  public final Timeout timeout = new Timeout(5, TimeUnit.SECONDS);

  @Test
  public void closeSuccess_withListenerRemove() throws Exception {

    CountDownLatch cdl = new CountDownLatch(1);

    Firestore fs = FirestoreOptions.getDefaultInstance().getService();
    ListenerRegistration listener = fs.collection("abcd").addSnapshotListener((value, error) -> {
      cdl.countDown();
    });

    fs.collection("abcd").document("doc1").set(ImmutableMap.of("foo", "bar")).get();

    cdl.await();
    listener.remove();
    fs.close();
  }

  @Test
  public void closeSuccess_withoutListenerRemove() throws Exception {

    CountDownLatch cdl = new CountDownLatch(1);

    Firestore fs = FirestoreOptions.getDefaultInstance().getService();
    ListenerRegistration listener = fs.collection("abcd").addSnapshotListener((value, error) -> {
      cdl.countDown();
    });

    fs.collection("abcd").document("doc1").set(ImmutableMap.of("foo", "bar")).get();

    cdl.await();
    fs.close();
  }

  @Test
  public void shutdownNowSuccess_withoutListenerRemove() throws Exception {

    CountDownLatch cdl = new CountDownLatch(1);

    Firestore fs = FirestoreOptions.getDefaultInstance().getService();
    ListenerRegistration listener = fs.collection("abcd").addSnapshotListener((value, error) -> {
      cdl.countDown();
    });

    fs.collection("abcd").document("doc1").set(ImmutableMap.of("foo", "bar")).get();

    cdl.await();
    fs.shutdownNow();
  }

  @Test
  public void shutdownSuccess_withoutListenerRemove() throws Exception {

    CountDownLatch cdl = new CountDownLatch(1);

    Firestore fs = FirestoreOptions.getDefaultInstance().getService();
    ListenerRegistration listener = fs.collection("abcd").addSnapshotListener((value, error) -> {
      cdl.countDown();
    });

    fs.collection("abcd").document("doc1").set(ImmutableMap.of("foo", "bar")).get();

    cdl.await();
    fs.shutdown();
  }
}

And I get these results when run against a build of your PR:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running io.github.benwhitehead.firestore.FirestoreCloseHangTest
Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 6.821 sec <<< FAILURE!
closeSuccess_withoutListenerRemove(io.github.benwhitehead.firestore.FirestoreCloseHangTest)  Time elapsed: 5.009 sec  <<< ERROR!
org.junit.runners.model.TestTimedOutException: test timed out after 5 seconds
        at sun.misc.Unsafe.park(Native Method)
        at java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:215)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedNanos(AbstractQueuedSynchronizer.java:1037)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(AbstractQueuedSynchronizer.java:1328)
        at java.util.concurrent.CountDownLatch.await(CountDownLatch.java:277)
        at io.grpc.internal.ManagedChannelImpl.awaitTermination(ManagedChannelImpl.java:897)
        at io.grpc.internal.ForwardingManagedChannel.awaitTermination(ForwardingManagedChannel.java:57)
        at com.google.api.gax.grpc.ChannelPool.awaitTermination(ChannelPool.java:207)
        at com.google.api.gax.grpc.GrpcTransportChannel.awaitTermination(GrpcTransportChannel.java:89)
        at com.google.api.gax.grpc.GrpcTransportChannel.close(GrpcTransportChannel.java:96)
        at com.google.cloud.firestore.spi.v1.GrpcFirestoreRpc.close(GrpcFirestoreRpc.java:139)
        at com.google.cloud.firestore.FirestoreImpl.close(FirestoreImpl.java:450)
        at io.github.benwhitehead.firestore.FirestoreCloseHangTest.closeSuccess_withoutListenerRemove(FirestoreCloseHangTest.java:48)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
        at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
        at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:288)
        at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:282)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.lang.Thread.run(Thread.java:748)


Results :

Tests in error: 
  closeSuccess_withoutListenerRemove(io.github.benwhitehead.firestore.FirestoreCloseHangTest): test timed out after 5 seconds

Based on these results the change to close didn't appear to force shutdown if a listener is still active.

[schmidt-sebastian]

I changed the behavior of close() to also call shutdown, which cancels any existing listeners. Your test should now pass. FWIW, I also added these tests to this PR.

[BenWhitehead]

A few minor items. It looks like some of the integration tests are failing due to a closed client, maybe a close is happening someplace too early or something.

[BenWhitehead]

I'm surprised this doesn't still call firestoreClient.close(). It looks like in GrpcFirestoreRpc you factored things to keep the existing behavior for close().

[schmidt-sebastian]

I don't have an opinion here - I do think it makes sense to keep the existing semantics, especially since we are adding a couple of new methods. If I call close() here the newly added shutdown tests time out though. Let me know if you want me to go back to calling close(), in which case I will remove the tests.

[BenWhitehead]

Yeah, I think keeping the existing semantic is probably beneficial.

For updating the test we may be able to do something like:

    ExecutorService testExecutorService = Executors.newSingleThreadExecutor();
    final Future<?> f = testExecutorService.submit(new Runnable() {
      @Override
      public void run() {
        firestore.close();
      }
    });
    
    try {
      f.get(5, TimeUnit.SECONDS);
      fail();
    } catch (TimeoutException e) {
      // expected
    } finally {
      testExecutorService.shutdown();
    }

[schmidt-sebastian]

Updated. Thanks for the reviews.

[BenWhitehead]

These changes are necessary becuse of grpc interrupting the channel while this could still be listenting?

[schmidt-sebastian]

Yes, and the exceptions thrown then do no have status codes. This is all based on local debugging.