diff --git a/core/src/main/java/io/grpc/internal/SharedResourceHolder.java b/core/src/main/java/io/grpc/internal/SharedResourceHolder.java index 40dbfeb9a58..93e53c696ca 100644 --- a/core/src/main/java/io/grpc/internal/SharedResourceHolder.java +++ b/core/src/main/java/io/grpc/internal/SharedResourceHolder.java @@ -128,8 +128,14 @@ synchronized T releaseInternal(final Resource resource, final T instance) // AppEngine must immediately release shared resources, particularly executors // which could retain request-scoped threads which become zombies after the request // completes. - resource.close(instance); - instances.remove(resource); + // We do not encourage exceptions to be thrown during close, but we would like it to + // be able to recover eventually and do not want future resource fetches reuse the broken + // one. + try { + resource.close(instance); + } finally { + instances.remove(resource); + } } else { Preconditions.checkState(cached.destroyTask == null, "Destroy task already scheduled"); // Schedule a delayed task to destroy the resource. @@ -142,11 +148,14 @@ public void run() { synchronized (SharedResourceHolder.this) { // Refcount may have gone up since the task was scheduled. Re-check it. if (cached.refcount == 0) { - resource.close(instance); - instances.remove(resource); - if (instances.isEmpty()) { - destroyer.shutdown(); - destroyer = null; + try { + resource.close(instance); + } finally { + instances.remove(resource); + if (instances.isEmpty()) { + destroyer.shutdown(); + destroyer = null; + } } } } diff --git a/core/src/test/java/io/grpc/internal/SharedResourceHolderTest.java b/core/src/test/java/io/grpc/internal/SharedResourceHolderTest.java index b4a991c8ec8..531632ca79c 100644 --- a/core/src/test/java/io/grpc/internal/SharedResourceHolderTest.java +++ b/core/src/test/java/io/grpc/internal/SharedResourceHolderTest.java @@ -173,6 +173,34 @@ public void close(ResourceInstance instance) { } } + @Test public void handleInstanceCloseError() { + class ExceptionOnCloseResource implements Resource { + @Override + public ResourceInstance create() { + return new ResourceInstance(); + } + + @Override + public void close(ResourceInstance instance) { + throw new RuntimeException(); + } + } + + Resource resource = new ExceptionOnCloseResource(); + ResourceInstance instance = holder.getInternal(resource); + holder.releaseInternal(resource, instance); + MockScheduledFuture scheduledDestroyTask = scheduledDestroyTasks.poll(); + try { + scheduledDestroyTask.runTask(); + fail("Should throw RuntimeException"); + } catch (RuntimeException e) { + // expected + } + + // Future resource fetches should not get the partially-closed one. + assertNotSame(instance, holder.getInternal(resource)); + } + private class MockExecutorFactory implements SharedResourceHolder.ScheduledExecutorFactory { @Override