feat: add shutdown() and shutdownNow()#673
Conversation
product-auto-label
bot
added
the
api: firestore
google-cla
bot
added
the
cla: yes
schmidt-sebastian
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| FirestoreStubSettings.newBuilder(clientContext) | ||
| .applyToAllUnaryMethods(retrySettingsSetter); | ||
| firestoreStub = GrpcFirestoreStub.create(firestoreBuilder.build()); | ||
| firestoreStub = GrpcFirestoreStub.create(clientContext); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I was able to add this back by adding the existing logic from close that manually closed the background resources.
schmidt-sebastian
added
the
kokoro:force-run
|
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: Based on these results the change to |
yoshi-kokoro
removed
the
kokoro:force-run
|
I changed the behavior of |
| @Override | ||
| public void close() throws Exception { | ||
| firestoreClient.close(); | ||
| shutdown(); |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
}There was a problem hiding this comment.
Updated. Thanks for the reviews.
| throwable, | ||
| GrpcStatusCode.of(getStatus(throwable).getCode()), | ||
| false))); | ||
| if (throwable instanceof FirestoreException) { |
There was a problem hiding this comment.
These changes are necessary becuse of grpc interrupting the channel while this could still be listenting?
There was a problem hiding this comment.
Yes, and the exceptions thrown then do no have status codes. This is all based on local debugging.
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java
Outdated
Show resolved
Hide resolved
3 participants
This adds a
shutdownand ashutdownNowmessage. Unfortunately, it probably requires a major version bump since the method is added to the main interface (did we make exceptions before?).