From 75980712b164c43001caf69f4365b845dad37e86 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 22 Jun 2021 14:49:27 -0600 Subject: [PATCH 01/12] feat: add shutdown() and shutdownNow() --- .../com/google/cloud/firestore/Firestore.java | 9 +++ .../google/cloud/firestore/FirestoreImpl.java | 12 ++++ .../com/google/cloud/firestore/Watch.java | 40 ++++++------ .../cloud/firestore/spi/v1/FirestoreRpc.java | 4 ++ .../firestore/spi/v1/GrpcFirestoreRpc.java | 27 ++++---- .../cloud/firestore/it/ITQueryWatchTest.java | 61 +++++++++++++++++-- 6 files changed, 115 insertions(+), 38 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java index 2cd897b49..5175fd1b4 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java @@ -296,4 +296,13 @@ void getAll( */ @Override void close() throws Exception; + + /** + * Initiates an orderly shutdown in which previously submitted work is finished, but no new work + * will be accepted. + */ + void shutdown(); + + /** Attempts to stop all actively executing work and halts the processing of waiting work. */ + void shutdownNow(); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 7a93e711d..a4ea4f0f2 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -451,6 +451,18 @@ public void close() throws Exception { closed = true; } + @Override + public void shutdown() { + firestoreClient.shutdown(); + closed = true; + } + + @Override + public void shutdownNow() { + firestoreClient.shutdownNow(); + closed = true; + } + private static class TransactionAsyncAdapter implements Transaction.AsyncFunction { private final Transaction.Function syncFunction; diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java index f355836f0..901cd8ac3 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java @@ -383,31 +383,36 @@ private void initStream() { new Runnable() { @Override public void run() { - if (!isActive.get()) { - return; - } - - synchronized (Watch.this) { + try { if (!isActive.get()) { return; } - Preconditions.checkState(stream == null); + synchronized (Watch.this) { + if (!isActive.get()) { + return; + } - current = false; - nextAttempt = backoff.createNextAttempt(nextAttempt); + Preconditions.checkState(stream == null); - Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_LISTEN); - stream = firestore.streamRequest(Watch.this, firestore.getClient().listenCallable()); + current = false; + nextAttempt = backoff.createNextAttempt(nextAttempt); - ListenRequest.Builder request = ListenRequest.newBuilder(); - request.setDatabase(firestore.getDatabaseName()); - request.setAddTarget(target); - if (resumeToken != null) { - request.getAddTargetBuilder().setResumeToken(resumeToken); - } + Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_LISTEN); + stream = + firestore.streamRequest(Watch.this, firestore.getClient().listenCallable()); - stream.onNext(request.build()); + ListenRequest.Builder request = ListenRequest.newBuilder(); + request.setDatabase(firestore.getDatabaseName()); + request.setAddTarget(target); + if (resumeToken != null) { + request.getAddTargetBuilder().setResumeToken(resumeToken); + } + + stream.onNext(request.build()); + } + } catch (Throwable throwable) { + onError(throwable); } } }, @@ -551,7 +556,6 @@ private static boolean isPermanentError(Throwable throwable) { switch (status.getCode()) { case CANCELLED: - case UNKNOWN: case DEADLINE_EXCEEDED: case RESOURCE_EXHAUSTED: case INTERNAL: diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/FirestoreRpc.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/FirestoreRpc.java index d31d6557c..c7fbcd3d9 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/FirestoreRpc.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/FirestoreRpc.java @@ -78,4 +78,8 @@ public interface FirestoreRpc extends AutoCloseable, ServiceRpc { /** Returns a bi-directional watch stream. */ BidiStreamingCallable listenCallable(); + + void shutdownNow(); + + void shutdown(); } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java index 692cb77b4..e4d64a6f6 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java @@ -16,7 +16,6 @@ package com.google.cloud.firestore.spi.v1; -import com.google.api.core.ApiFunction; import com.google.api.gax.core.BackgroundResource; import com.google.api.gax.core.GaxProperties; import com.google.api.gax.grpc.GrpcCallContext; @@ -27,8 +26,6 @@ import com.google.api.gax.rpc.NoHeaderProvider; import com.google.api.gax.rpc.ServerStreamingCallable; import com.google.api.gax.rpc.TransportChannel; -import com.google.api.gax.rpc.UnaryCallSettings; -import com.google.api.gax.rpc.UnaryCallSettings.Builder; import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.NoCredentials; import com.google.cloud.ServiceOptions; @@ -38,7 +35,6 @@ import com.google.cloud.firestore.v1.FirestoreClient.ListDocumentsPagedResponse; import com.google.cloud.firestore.v1.FirestoreSettings; import com.google.cloud.firestore.v1.stub.FirestoreStub; -import com.google.cloud.firestore.v1.stub.FirestoreStubSettings; import com.google.cloud.firestore.v1.stub.GrpcFirestoreStub; import com.google.cloud.grpc.GrpcTransportOptions; import com.google.cloud.grpc.GrpcTransportOptions.ExecutorFactory; @@ -126,18 +122,7 @@ public GrpcFirestoreRpc(final FirestoreOptions options) throws IOException { clientContext = ClientContext.create(settingsBuilder.build()); } - ApiFunction, Void> retrySettingsSetter = - new ApiFunction, Void>() { - @Override - public Void apply(UnaryCallSettings.Builder builder) { - builder.setRetrySettings(options.getRetrySettings()); - return null; - } - }; - FirestoreStubSettings.Builder firestoreBuilder = - FirestoreStubSettings.newBuilder(clientContext) - .applyToAllUnaryMethods(retrySettingsSetter); - firestoreStub = GrpcFirestoreStub.create(firestoreBuilder.build()); + firestoreStub = GrpcFirestoreStub.create(clientContext); } catch (Exception e) { throw new IOException(e); } @@ -219,6 +204,16 @@ public BidiStreamingCallable listenCallable() { return firestoreStub.listenCallable(); } + @Override + public void shutdown() { + firestoreStub.shutdown(); + } + + @Override + public void shutdownNow() { + firestoreStub.shutdownNow(); + } + // This class is needed solely to get access to protected method setInternalHeaderProvider() private static class FirestoreSettingsBuilder extends FirestoreSettings.Builder { private FirestoreSettingsBuilder(FirestoreSettings settings) { diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index cb9d69ec1..f2c03fd88 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -384,6 +384,21 @@ public void limitToLast() throws Exception { listenerAssertions.addedIdsIsAnyOf(emptyList(), asList("doc2", "doc3")); } + @Test + public void shutdownNowPreventsListener() throws Exception { + Query query = randomColl.whereEqualTo("foo", "bar"); + QuerySnapshotEventListener listener = + QuerySnapshotEventListener.builder().setExpectError().build(); + + firestore.shutdownNow(); + query.addSnapshotListener(listener); + + listener.eventsCountDownLatch.awaitError(); + + ListenerAssertions listenerAssertions = listener.assertions(); + listenerAssertions.hasError(); + } + /** * A tuple class used by {@code #queryWatch}. This class represents an event delivered to the * registered query listener. @@ -402,6 +417,7 @@ private static final class ListenerEvent { private static final class EventsCountDownLatch { private final CountDownLatch initialEventsCountDownLatch; private final int initialEventCount; + private final CountDownLatch errorCountDownLatch; private final EnumMap eventsCounts; private final EnumMap eventsCountDownLatches; @@ -409,9 +425,11 @@ private static final class EventsCountDownLatch { int initialEventCount, int addedInitialCount, int modifiedInitialCount, - int removedInitialCount) { + int removedInitialCount, + int errorCount) { initialEventsCountDownLatch = new CountDownLatch(initialEventCount); this.initialEventCount = initialEventCount; + this.errorCountDownLatch = new CountDownLatch(errorCount); eventsCounts = new EnumMap<>(DocumentChange.Type.class); eventsCounts.put(DocumentChange.Type.ADDED, addedInitialCount); eventsCounts.put(DocumentChange.Type.MODIFIED, modifiedInitialCount); @@ -432,10 +450,18 @@ void countDown(DocumentChange.Type type) { eventsCountDownLatches.get(type).countDown(); } + void countError() { + errorCountDownLatch.countDown(); + } + void awaitInitialEvents() throws InterruptedException { initialEventsCountDownLatch.await(5 * initialEventCount, TimeUnit.SECONDS); } + void awaitError() throws InterruptedException { + errorCountDownLatch.await(5, TimeUnit.SECONDS); + } + void await(DocumentChange.Type type) throws InterruptedException { int count = eventsCounts.get(type); eventsCountDownLatches.get(type).await(5 * count, TimeUnit.SECONDS); @@ -447,11 +473,15 @@ static class QuerySnapshotEventListener implements EventListener final EventsCountDownLatch eventsCountDownLatch; private QuerySnapshotEventListener( - int initialCount, int addedEventCount, int modifiedEventCount, int removedEventCount) { + int initialCount, + int addedEventCount, + int modifiedEventCount, + int removedEventCount, + int errorCount) { this.receivedEvents = Collections.synchronizedList(new ArrayList()); this.eventsCountDownLatch = new EventsCountDownLatch( - initialCount, addedEventCount, modifiedEventCount, removedEventCount); + initialCount, addedEventCount, modifiedEventCount, removedEventCount, errorCount); } @Override @@ -463,6 +493,9 @@ public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException eventsCountDownLatch.countDown(docChange.getType()); } } + if (error != null) { + eventsCountDownLatch.countError(); + } eventsCountDownLatch.countDown(); } @@ -480,6 +513,7 @@ static final class Builder { private int addedEventCount = 0; private int modifiedEventCount = 0; private int removedEventCount = 0; + private int errorCount = 0; private Builder() {} @@ -503,9 +537,14 @@ Builder setRemovedEventCount(int removedEventCount) { return this; } + Builder setExpectError() { + this.errorCount = 1; + return this; + } + public QuerySnapshotEventListener build() { return new QuerySnapshotEventListener( - initialEventCount, addedEventCount, modifiedEventCount, removedEventCount); + initialEventCount, addedEventCount, modifiedEventCount, removedEventCount, errorCount); } } @@ -536,6 +575,20 @@ public boolean apply(ListenerEvent input) { assertWithMessage("snapshotListener received an error").that(anyError).isAbsent(); } + private void hasError() { + final Optional anyError = + events.firstMatch( + new Predicate() { + @Override + public boolean apply(ListenerEvent input) { + return input.error != null; + } + }); + assertWithMessage("snapshotListener did not receive an expected error") + .that(anyError) + .isPresent(); + } + private static List getQuerySnapshots(FluentIterable events) { return events .filter( From 11ceaf06cd91634be3696add79952bcdb16589ee Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 10:11:37 -0600 Subject: [PATCH 02/12] Nullable status --- .../com/google/cloud/firestore/Watch.java | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java index 901cd8ac3..7eda432cb 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java @@ -337,15 +337,18 @@ private void closeStream(final Throwable throwable) { new Runnable() { @Override public void run() { - listener.onEvent( - null, - throwable instanceof FirestoreException - ? (FirestoreException) throwable - : FirestoreException.forApiException( - new ApiException( - throwable, - GrpcStatusCode.of(getStatus(throwable).getCode()), - false))); + if (throwable instanceof FirestoreException) { + listener.onEvent(null, (FirestoreException) throwable); + } else { + Status status = getStatus(throwable); + FirestoreException firestoreException = + FirestoreException.forApiException( + new ApiException( + throwable, + GrpcStatusCode.of(status != null ? status.getCode() : Code.UNKNOWN), + false)); + listener.onEvent(null, firestoreException); + } } }); } @@ -554,8 +557,13 @@ private List computeSnapshot(Timestamp readTime) { private static boolean isPermanentError(Throwable throwable) { Status status = getStatus(throwable); + if (status == null) { + return true; + } + switch (status.getCode()) { case CANCELLED: + case UNKNOWN: case DEADLINE_EXCEEDED: case RESOURCE_EXHAUSTED: case INTERNAL: @@ -567,20 +575,20 @@ private static boolean isPermanentError(Throwable throwable) { } } - /** Extracts the GRPC status code if available. Returns UNKNOWN for non-GRPC exceptions. */ + /** Extracts the GRPC status code if available. Returns `null` for non-GRPC exceptions. */ + @Nullable private static Status getStatus(Throwable throwable) { - Status status = Status.UNKNOWN; - if (throwable instanceof StatusRuntimeException) { - status = ((StatusRuntimeException) throwable).getStatus(); + return ((StatusRuntimeException) throwable).getStatus(); } else if (throwable instanceof StatusException) { - status = ((StatusException) throwable).getStatus(); + return ((StatusException) throwable).getStatus(); } - return status; + return null; } /** Determines whether we need to initiate a longer backoff due to system overload. */ private static boolean isResourceExhaustedError(Throwable throwable) { - return getStatus(throwable).getCode().equals(Code.RESOURCE_EXHAUSTED); + Status status = getStatus(throwable); + return status != null && status.getCode().equals(Code.RESOURCE_EXHAUSTED); }; } From 2e032f2fd4b136fc8b27c214378a8d8374441300 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 10:14:11 -0600 Subject: [PATCH 03/12] Test comment --- .../java/com/google/cloud/firestore/it/ITQueryWatchTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index f2c03fd88..170753eaf 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -390,6 +390,10 @@ public void shutdownNowPreventsListener() throws Exception { QuerySnapshotEventListener listener = QuerySnapshotEventListener.builder().setExpectError().build(); + // While a better test would test a shutdown after the listener has been added, this behavior + // leads to flakes as the timing of both the Watch stream and the shutdown call influence + // whether an error is raised. In some circumstances, the stream drops without any + // notifications. firestore.shutdownNow(); query.addSnapshotListener(listener); From fc77332d26c4a832a076be721c25f6c773fb57ec Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 17:57:34 -0600 Subject: [PATCH 04/12] Review --- .../clirr-ignored-differences.xml | 21 ++++ .../google/cloud/firestore/FirestoreImpl.java | 3 +- .../firestore/spi/v1/GrpcFirestoreRpc.java | 59 +++++++--- .../cloud/firestore/it/ITQueryWatchTest.java | 21 +++- .../cloud/firestore/it/ITShutdownTest.java | 111 ++++++++++++++++++ 5 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java diff --git a/google-cloud-firestore/clirr-ignored-differences.xml b/google-cloud-firestore/clirr-ignored-differences.xml index d32596ef0..bf989e438 100644 --- a/google-cloud-firestore/clirr-ignored-differences.xml +++ b/google-cloud-firestore/clirr-ignored-differences.xml @@ -17,6 +17,27 @@ + + + 7012 + com/google/cloud/firestore/Firestore + void shutdown() + + + 7012 + com/google/cloud/firestore/Firestore + void shutdownNow() + + + 7012 + com/google/cloud/firestore/spi/v1/FirestoreRpc + void shutdown() + + + 7012 + com/google/cloud/firestore/spi/v1/FirestoreRpc + void shutdownNow() + diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index a4ea4f0f2..bb99a53c7 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -447,8 +447,7 @@ public FirestoreOptions getOptions() { @Override public void close() throws Exception { - firestoreClient.close(); - closed = true; + shutdown(); } @Override diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java index e4d64a6f6..edd547130 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/spi/v1/GrpcFirestoreRpc.java @@ -16,6 +16,7 @@ package com.google.cloud.firestore.spi.v1; +import com.google.api.core.ApiFunction; import com.google.api.gax.core.BackgroundResource; import com.google.api.gax.core.GaxProperties; import com.google.api.gax.grpc.GrpcCallContext; @@ -26,6 +27,7 @@ import com.google.api.gax.rpc.NoHeaderProvider; import com.google.api.gax.rpc.ServerStreamingCallable; import com.google.api.gax.rpc.TransportChannel; +import com.google.api.gax.rpc.UnaryCallSettings; import com.google.api.gax.rpc.UnaryCallable; import com.google.cloud.NoCredentials; import com.google.cloud.ServiceOptions; @@ -35,6 +37,7 @@ import com.google.cloud.firestore.v1.FirestoreClient.ListDocumentsPagedResponse; import com.google.cloud.firestore.v1.FirestoreSettings; import com.google.cloud.firestore.v1.stub.FirestoreStub; +import com.google.cloud.firestore.v1.stub.FirestoreStubSettings; import com.google.cloud.firestore.v1.stub.GrpcFirestoreStub; import com.google.cloud.grpc.GrpcTransportOptions; import com.google.cloud.grpc.GrpcTransportOptions.ExecutorFactory; @@ -122,7 +125,18 @@ public GrpcFirestoreRpc(final FirestoreOptions options) throws IOException { clientContext = ClientContext.create(settingsBuilder.build()); } - firestoreStub = GrpcFirestoreStub.create(clientContext); + ApiFunction, Void> retrySettingsSetter = + new ApiFunction, Void>() { + @Override + public Void apply(UnaryCallSettings.Builder builder) { + builder.setRetrySettings(options.getRetrySettings()); + return null; + } + }; + FirestoreStubSettings.Builder firestoreBuilder = + FirestoreStubSettings.newBuilder(clientContext) + .applyToAllUnaryMethods(retrySettingsSetter); + firestoreStub = GrpcFirestoreStub.create(firestoreBuilder.build()); } catch (Exception e) { throw new IOException(e); } @@ -130,18 +144,43 @@ public GrpcFirestoreRpc(final FirestoreOptions options) throws IOException { @Override public void close() throws Exception { + if (!closed) { + firestoreStub.close(); + for (BackgroundResource resource : clientContext.getBackgroundResources()) { + resource.close(); + } + executorFactory.release(executor); + closed = true; + } + for (BackgroundResource resource : clientContext.getBackgroundResources()) { + resource.awaitTermination(1, TimeUnit.SECONDS); + } + } + + @Override + public void shutdown() { if (closed) { return; } - closed = true; - firestoreStub.close(); + firestoreStub.shutdown(); for (BackgroundResource resource : clientContext.getBackgroundResources()) { - resource.close(); + resource.shutdown(); } + executorFactory.release(executor); + closed = true; + } + + @Override + public void shutdownNow() { + if (closed) { + return; + } + firestoreStub.shutdownNow(); for (BackgroundResource resource : clientContext.getBackgroundResources()) { - resource.awaitTermination(1, TimeUnit.SECONDS); + resource.shutdownNow(); } executorFactory.release(executor); + closed = true; } @Override @@ -204,16 +243,6 @@ public BidiStreamingCallable listenCallable() { return firestoreStub.listenCallable(); } - @Override - public void shutdown() { - firestoreStub.shutdown(); - } - - @Override - public void shutdownNow() { - firestoreStub.shutdownNow(); - } - // This class is needed solely to get access to protected method setInternalHeaderProvider() private static class FirestoreSettingsBuilder extends FirestoreSettings.Builder { private FirestoreSettingsBuilder(FirestoreSettings settings) { diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index 170753eaf..a99c77286 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -384,16 +384,27 @@ public void limitToLast() throws Exception { listenerAssertions.addedIdsIsAnyOf(emptyList(), asList("doc2", "doc3")); } + @Test + public void shutdownNowRejectsListener() throws Exception { + Query query = randomColl.whereEqualTo("foo", "bar"); + QuerySnapshotEventListener listener = + QuerySnapshotEventListener.builder().setExpectError().build(); + + query.addSnapshotListener(listener); + firestore.shutdownNow(); + + listener.eventsCountDownLatch.awaitError(); + + ListenerAssertions listenerAssertions = listener.assertions(); + listenerAssertions.hasError(); + } + @Test public void shutdownNowPreventsListener() throws Exception { Query query = randomColl.whereEqualTo("foo", "bar"); QuerySnapshotEventListener listener = - QuerySnapshotEventListener.builder().setExpectError().build(); + QuerySnapshotEventListener.builder().setExpectError().build(); - // While a better test would test a shutdown after the listener has been added, this behavior - // leads to flakes as the timing of both the Watch stream and the shutdown call influence - // whether an error is raised. In some circumstances, the stream drops without any - // notifications. firestore.shutdownNow(); query.addSnapshotListener(listener); diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java new file mode 100644 index 000000000..507d5bf7e --- /dev/null +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -0,0 +1,111 @@ +package com.google.cloud.firestore.it; + +import com.google.cloud.firestore.EventListener; +import com.google.cloud.firestore.Firestore; +import com.google.cloud.firestore.FirestoreException; +import com.google.cloud.firestore.FirestoreOptions; +import com.google.cloud.firestore.ListenerRegistration; +import com.google.cloud.firestore.QuerySnapshot; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.Timeout; + +import javax.annotation.Nullable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class ITShutdownTest { + @Rule + public final Timeout timeout = new Timeout(5, TimeUnit.SECONDS); + + @Test + public void closeSuccess_withListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { + @Override + public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + + cdl.await(); + listener.remove(); + fs.close(); + } + + @Test + public void closeSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { + @Override + public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.close(); + } + + @Test + public void shutdownNowSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { + @Override + public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + + cdl.await(); + fs.shutdownNow(); + } + + @Test + public void shutdownSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { + @Override + public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.shutdown(); + } + + @Test + public void closeAndShutdown() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { + @Override + public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + + cdl.await(); + fs.close(); + fs.shutdown(); + fs.shutdownNow(); + } +} From f2c76a15061984327547715401c93e17f9838113 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 20:57:05 -0600 Subject: [PATCH 05/12] Formatting --- .../google/cloud/firestore/FirestoreImpl.java | 2 +- .../cloud/firestore/it/ITQueryWatchTest.java | 4 +- .../cloud/firestore/it/ITShutdownTest.java | 209 ++++++++++-------- 3 files changed, 115 insertions(+), 100 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index bb99a53c7..bcc47df0f 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -447,7 +447,7 @@ public FirestoreOptions getOptions() { @Override public void close() throws Exception { - shutdown(); + shutdown(); } @Override diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index a99c77286..e95121676 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -388,7 +388,7 @@ public void limitToLast() throws Exception { public void shutdownNowRejectsListener() throws Exception { Query query = randomColl.whereEqualTo("foo", "bar"); QuerySnapshotEventListener listener = - QuerySnapshotEventListener.builder().setExpectError().build(); + QuerySnapshotEventListener.builder().setExpectError().build(); query.addSnapshotListener(listener); firestore.shutdownNow(); @@ -403,7 +403,7 @@ public void shutdownNowRejectsListener() throws Exception { public void shutdownNowPreventsListener() throws Exception { Query query = randomColl.whereEqualTo("foo", "bar"); QuerySnapshotEventListener listener = - QuerySnapshotEventListener.builder().setExpectError().build(); + QuerySnapshotEventListener.builder().setExpectError().build(); firestore.shutdownNow(); query.addSnapshotListener(listener); diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java index 507d5bf7e..933f66622 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -6,106 +6,121 @@ import com.google.cloud.firestore.FirestoreOptions; import com.google.cloud.firestore.ListenerRegistration; import com.google.cloud.firestore.QuerySnapshot; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.Timeout; -import javax.annotation.Nullable; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; - public class ITShutdownTest { - @Rule - public final Timeout timeout = new Timeout(5, TimeUnit.SECONDS); - - @Test - public void closeSuccess_withListenerRemove() throws Exception { - - final CountDownLatch cdl = new CountDownLatch(1); - - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { - @Override - public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - - cdl.await(); - listener.remove(); - fs.close(); - } - - @Test - public void closeSuccess_withoutListenerRemove() throws Exception { - - final CountDownLatch cdl = new CountDownLatch(1); - - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { - @Override - public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); - fs.close(); - } - - @Test - public void shutdownNowSuccess_withoutListenerRemove() throws Exception { - - final CountDownLatch cdl = new CountDownLatch(1); - - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { - @Override - public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - - cdl.await(); - fs.shutdownNow(); - } - - @Test - public void shutdownSuccess_withoutListenerRemove() throws Exception { - - final CountDownLatch cdl = new CountDownLatch(1); - - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { - @Override - public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); - fs.shutdown(); - } - - @Test - public void closeAndShutdown() throws Exception { - - final CountDownLatch cdl = new CountDownLatch(1); - - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = fs.collection("abcd").addSnapshotListener(new EventListener() { - @Override - public void onEvent(@Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - - cdl.await(); - fs.close(); - fs.shutdown(); - fs.shutdownNow(); - } + @Rule public final Timeout timeout = new Timeout(5, TimeUnit.SECONDS); + + @Test + public void closeSuccess_withListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = + fs.collection("abcd") + .addSnapshotListener( + new EventListener() { + @Override + public void onEvent( + @Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + listener.remove(); + fs.close(); + } + + @Test + public void closeSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = + fs.collection("abcd") + .addSnapshotListener( + new EventListener() { + @Override + public void onEvent( + @Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.close(); + } + + @Test + public void shutdownNowSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = + fs.collection("abcd") + .addSnapshotListener( + new EventListener() { + @Override + public void onEvent( + @Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.shutdownNow(); + } + + @Test + public void shutdownSuccess_withoutListenerRemove() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = + fs.collection("abcd") + .addSnapshotListener( + new EventListener() { + @Override + public void onEvent( + @Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.shutdown(); + } + + @Test + public void closeAndShutdown() throws Exception { + + final CountDownLatch cdl = new CountDownLatch(1); + + Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + ListenerRegistration listener = + fs.collection("abcd") + .addSnapshotListener( + new EventListener() { + @Override + public void onEvent( + @Nullable QuerySnapshot value, @Nullable FirestoreException error) { + cdl.countDown(); + } + }); + + cdl.await(); + fs.close(); + fs.shutdown(); + fs.shutdownNow(); + } } From f416d061f6650f9da1871699b1effa212fe87f70 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 21:01:34 -0600 Subject: [PATCH 06/12] Copyright --- .../cloud/firestore/it/ITShutdownTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java index 933f66622..d5832b008 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.firestore.it; import com.google.cloud.firestore.EventListener; From ae5bd94e30b6f258efbb074e96e7f0aa3d313ca8 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 21:02:00 -0600 Subject: [PATCH 07/12] Remove spaces --- .../java/com/google/cloud/firestore/it/ITShutdownTest.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java index d5832b008..6e86b983b 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -34,7 +34,6 @@ public class ITShutdownTest { @Test public void closeSuccess_withListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); Firestore fs = FirestoreOptions.getDefaultInstance().getService(); @@ -56,7 +55,6 @@ public void onEvent( @Test public void closeSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); Firestore fs = FirestoreOptions.getDefaultInstance().getService(); @@ -77,7 +75,6 @@ public void onEvent( @Test public void shutdownNowSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); Firestore fs = FirestoreOptions.getDefaultInstance().getService(); @@ -98,7 +95,6 @@ public void onEvent( @Test public void shutdownSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); Firestore fs = FirestoreOptions.getDefaultInstance().getService(); @@ -119,7 +115,6 @@ public void onEvent( @Test public void closeAndShutdown() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); Firestore fs = FirestoreOptions.getDefaultInstance().getService(); From 734bd7e4e450c10b7e689e11a7a30871680b28c0 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 23 Jun 2021 21:07:13 -0600 Subject: [PATCH 08/12] Add test helper --- .../cloud/firestore/it/ITShutdownTest.java | 84 +++++-------------- 1 file changed, 20 insertions(+), 64 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java index 6e86b983b..3d3bf6a87 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -21,105 +21,64 @@ import com.google.cloud.firestore.FirestoreException; import com.google.cloud.firestore.FirestoreOptions; import com.google.cloud.firestore.ListenerRegistration; +import com.google.cloud.firestore.LocalFirestoreHelper; import com.google.cloud.firestore.QuerySnapshot; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestName; import org.junit.rules.Timeout; public class ITShutdownTest { @Rule public final Timeout timeout = new Timeout(5, TimeUnit.SECONDS); + @Rule public TestName testName = new TestName(); @Test public void closeSuccess_withListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = - fs.collection("abcd") - .addSnapshotListener( - new EventListener() { - @Override - public void onEvent( - @Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); + ListenerRegistration listener = attachListener(fs); listener.remove(); fs.close(); } @Test public void closeSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = - fs.collection("abcd") - .addSnapshotListener( - new EventListener() { - @Override - public void onEvent( - @Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); + attachListener(fs); fs.close(); } @Test public void shutdownNowSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = - fs.collection("abcd") - .addSnapshotListener( - new EventListener() { - @Override - public void onEvent( - @Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); + attachListener(fs); fs.shutdownNow(); } @Test public void shutdownSuccess_withoutListenerRemove() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = - fs.collection("abcd") - .addSnapshotListener( - new EventListener() { - @Override - public void onEvent( - @Nullable QuerySnapshot value, @Nullable FirestoreException error) { - cdl.countDown(); - } - }); - - cdl.await(); + attachListener(fs); fs.shutdown(); } @Test public void closeAndShutdown() throws Exception { - final CountDownLatch cdl = new CountDownLatch(1); - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); - ListenerRegistration listener = - fs.collection("abcd") + attachListener(fs); + fs.close(); + fs.shutdown(); + fs.shutdownNow(); + } + + private ListenerRegistration attachListener(Firestore fs) throws InterruptedException { + final CountDownLatch cdl = new CountDownLatch(1); + ListenerRegistration listenerRegistration = + fs.collection( + String.format( + "java-%s-%s", testName.getMethodName(), LocalFirestoreHelper.autoId())) .addSnapshotListener( new EventListener() { @Override @@ -128,10 +87,7 @@ public void onEvent( cdl.countDown(); } }); - cdl.await(); - fs.close(); - fs.shutdown(); - fs.shutdownNow(); + return listenerRegistration; } } From 3f85e6447457da6e90b995c8775af272255d692a Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 28 Jun 2021 13:25:12 -0600 Subject: [PATCH 09/12] Fix integration tests --- .../cloud/firestore/it/ITQueryWatchTest.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index e95121676..5b201e3a0 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -54,6 +54,8 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; + +import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -69,21 +71,17 @@ public final class ITQueryWatchTest { private CollectionReference randomColl; - @BeforeClass - public static void beforeClass() { - FirestoreOptions firestoreOptions = FirestoreOptions.newBuilder().build(); - firestore = firestoreOptions.getService(); - } - @Before public void before() { + FirestoreOptions firestoreOptions = FirestoreOptions.newBuilder().build(); + firestore = firestoreOptions.getService(); String autoId = LocalFirestoreHelper.autoId(); String collPath = String.format("java-%s-%s", testName.getMethodName(), autoId); randomColl = firestore.collection(collPath); } - @AfterClass - public static void afterClass() throws Exception { + @After + public void after() throws Exception { Preconditions.checkNotNull( firestore, "Error instantiating Firestore. Check that the service account credentials were properly set."); From 060a6cabd06b9f7f035b2532816e007b73433ebe Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 28 Jun 2021 13:33:35 -0600 Subject: [PATCH 10/12] Test renames --- .../java/com/google/cloud/firestore/it/ITQueryWatchTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index 5b201e3a0..09fc91902 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -383,7 +383,7 @@ public void limitToLast() throws Exception { } @Test - public void shutdownNowRejectsListener() throws Exception { + public void shutdownNowTerminatesActiveListener() throws Exception { Query query = randomColl.whereEqualTo("foo", "bar"); QuerySnapshotEventListener listener = QuerySnapshotEventListener.builder().setExpectError().build(); @@ -398,7 +398,7 @@ public void shutdownNowRejectsListener() throws Exception { } @Test - public void shutdownNowPreventsListener() throws Exception { + public void shutdownNowPreventsAddingNewListener() throws Exception { Query query = randomColl.whereEqualTo("foo", "bar"); QuerySnapshotEventListener listener = QuerySnapshotEventListener.builder().setExpectError().build(); From bdd64df6bed9c16a771d8d74d9e7fc3034edc0a5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 28 Jun 2021 19:18:39 -0600 Subject: [PATCH 11/12] Format --- .../java/com/google/cloud/firestore/it/ITQueryWatchTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java index 09fc91902..c053af510 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java @@ -54,11 +54,8 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; - import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestName; From 903f7941dd8d0328c7d18f27167a585cb819e573 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 29 Jun 2021 10:26:38 -0600 Subject: [PATCH 12/12] Use close() again --- .../google/cloud/firestore/FirestoreImpl.java | 3 +- .../cloud/firestore/it/ITShutdownTest.java | 37 +++++++++++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index bcc47df0f..a4ea4f0f2 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -447,7 +447,8 @@ public FirestoreOptions getOptions() { @Override public void close() throws Exception { - shutdown(); + firestoreClient.close(); + closed = true; } @Override diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java index 3d3bf6a87..b6e9e87d5 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITShutdownTest.java @@ -16,6 +16,9 @@ package com.google.cloud.firestore.it; +import static org.junit.Assert.fail; + +import com.google.api.core.SettableApiFuture; import com.google.cloud.firestore.EventListener; import com.google.cloud.firestore.Firestore; import com.google.cloud.firestore.FirestoreException; @@ -24,7 +27,10 @@ import com.google.cloud.firestore.LocalFirestoreHelper; import com.google.cloud.firestore.QuerySnapshot; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; @@ -44,10 +50,33 @@ public void closeSuccess_withListenerRemove() throws Exception { } @Test - public void closeSuccess_withoutListenerRemove() throws Exception { - Firestore fs = FirestoreOptions.getDefaultInstance().getService(); + public void closeFailure_withoutListenerRemove() throws Exception { + final Firestore fs = FirestoreOptions.getDefaultInstance().getService(); attachListener(fs); - fs.close(); + + ExecutorService testExecutorService = Executors.newSingleThreadExecutor(); + final SettableApiFuture result = SettableApiFuture.create(); + testExecutorService.submit( + new Runnable() { + @Override + public void run() { + try { + fs.close(); + result.set(null); + } catch (Throwable throwable) { + result.setException(throwable); + } + } + }); + + try { + result.get(1, TimeUnit.SECONDS); + fail(); + } catch (TimeoutException e) { + // Expected + } finally { + testExecutorService.shutdown(); + } } @Test @@ -68,9 +97,9 @@ public void shutdownSuccess_withoutListenerRemove() throws Exception { public void closeAndShutdown() throws Exception { Firestore fs = FirestoreOptions.getDefaultInstance().getService(); attachListener(fs); - fs.close(); fs.shutdown(); fs.shutdownNow(); + fs.close(); } private ListenerRegistration attachListener(Firestore fs) throws InterruptedException {