From 726cc6a7fd18d67d591693838b08b994812bfaeb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Jan 2018 17:37:47 -0800 Subject: [PATCH 1/3] Adding .close() to Firestore Service --- .../java/com/google/cloud/firestore/Firestore.java | 10 +++++++++- .../com/google/cloud/firestore/FirestoreImpl.java | 14 ++++++++++++++ .../google/cloud/firestore/it/ITSystemTest.java | 6 ++++++ 3 files changed, 29 insertions(+), 1 deletion(-) 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 663f7297d050..49ae9b1aaa4c 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 @@ -22,7 +22,7 @@ import javax.annotation.Nonnull; /** Represents a Firestore Database and is the entry point for all Firestore operations */ -public interface Firestore extends Service { +public interface Firestore extends Service, AutoCloseable { /** * Gets a {@link CollectionReference} that refers to the collection at the specified path. @@ -91,4 +91,12 @@ ApiFuture runTransaction( */ @Nonnull WriteBatch batch(); + + /** + * Closes the gRPC channels associated with this instance and frees up their resources. This + * method blocks until all channels are closed. Once this method is called, this Firestore client + * is no longer usable. + */ + @Override + void close() throws Exception; } 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 47248568fac7..04aa9b92599f 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 @@ -30,13 +30,16 @@ import com.google.firestore.v1beta1.BatchGetDocumentsResponse; import com.google.firestore.v1beta1.DatabaseRootName; import com.google.protobuf.ByteString; +import io.grpc.ManagedChannel; import io.grpc.Status; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Random; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; +import java.util.logging.Level; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.threeten.bp.Instant; @@ -66,6 +69,8 @@ static String autoId() { private final FirestoreOptions firestoreOptions; private final ResourcePath databasePath; + private boolean closed = false; + FirestoreImpl(FirestoreOptions options) { this(options, options.getFirestoreRpc()); } @@ -324,6 +329,7 @@ FirestoreRpc getClient() { /** Request funnel for all read/write requests. */ ApiFuture sendRequest( RequestT requestT, UnaryCallable callable) { + Preconditions.checkState(!closed, "Firestore client has already been closed"); return callable.futureCall(requestT); } @@ -332,6 +338,7 @@ void streamRequest( RequestT requestT, ApiStreamObserver responseObserverT, ServerStreamingCallable callable) { + Preconditions.checkState(!closed, "Firestore client has already been closed"); callable.serverStreamingCall(requestT, responseObserverT); } @@ -339,6 +346,7 @@ void streamRequest( ApiStreamObserver streamRequest( ApiStreamObserver responseObserverT, BidiStreamingCallable callable) { + Preconditions.checkState(!closed, "Firestore client has already been closed"); return callable.bidiStreamingCall(responseObserverT); } @@ -346,4 +354,10 @@ ApiStreamObserver streamRequest( public FirestoreOptions getOptions() { return firestoreOptions; } + + @Override + public void close() throws Exception { + closed = true; + firestoreClient.close(); + } } diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index 132b414f8279..13636490a58a 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -62,6 +62,7 @@ import java.util.concurrent.Semaphore; import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nullable; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -92,6 +93,11 @@ public void before() { randomDoc = randomColl.document(); } + @After + public void after() throws Exception { + firestore.close(); + } + private DocumentReference addDocument(String key, Object value, Object... fields) throws Exception { DocumentReference documentReference = randomColl.document(); From 07e7aae1de96a6fb1425c9cbf6a6db40484fee40 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Jan 2018 17:45:47 -0800 Subject: [PATCH 2/3] Marking closed only when firestoreClient.close() didn't throw. --- .../src/main/java/com/google/cloud/firestore/FirestoreImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 04aa9b92599f..f74001243b56 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 @@ -357,7 +357,7 @@ public FirestoreOptions getOptions() { @Override public void close() throws Exception { - closed = true; firestoreClient.close(); + closed = true; } } From 236878c6b21dae4da701ac1e0dd27785a4769539 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Jan 2018 17:47:35 -0800 Subject: [PATCH 3/3] Lint errors. --- .../google/cloud/firestore/FirestoreImpl.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 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 f74001243b56..4df571b02cea 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 @@ -30,16 +30,13 @@ import com.google.firestore.v1beta1.BatchGetDocumentsResponse; import com.google.firestore.v1beta1.DatabaseRootName; import com.google.protobuf.ByteString; -import io.grpc.ManagedChannel; import io.grpc.Status; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Random; -import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; -import java.util.logging.Level; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.threeten.bp.Instant; @@ -55,21 +52,11 @@ class FirestoreImpl implements Firestore { private static final String AUTO_ID_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; - /** Creates a pseudo-random 20-character ID that can be used for Firestore documents. */ - static String autoId() { - StringBuilder builder = new StringBuilder(); - int maxRandom = AUTO_ID_ALPHABET.length(); - for (int i = 0; i < AUTO_ID_LENGTH; i++) { - builder.append(AUTO_ID_ALPHABET.charAt(RANDOM.nextInt(maxRandom))); - } - return builder.toString(); - } - private final FirestoreRpc firestoreClient; private final FirestoreOptions firestoreOptions; private final ResourcePath databasePath; - private boolean closed = false; + private boolean closed; FirestoreImpl(FirestoreOptions options) { this(options, options.getFirestoreRpc()); @@ -86,6 +73,16 @@ static String autoId() { ResourcePath.create(DatabaseRootName.of(options.getProjectId(), options.getDatabaseId())); } + /** Creates a pseudo-random 20-character ID that can be used for Firestore documents. */ + static String autoId() { + StringBuilder builder = new StringBuilder(); + int maxRandom = AUTO_ID_ALPHABET.length(); + for (int i = 0; i < AUTO_ID_LENGTH; i++) { + builder.append(AUTO_ID_ALPHABET.charAt(RANDOM.nextInt(maxRandom))); + } + return builder.toString(); + } + @Nonnull @Override public WriteBatch batch() {