From 09b1446278fe39c8ad3060234711eb17a03f1615 Mon Sep 17 00:00:00 2001 From: jaymode Date: Tue, 7 May 2019 14:18:50 -0600 Subject: [PATCH] Remove close method in PageCacheRecycler/Recycler The changes in #39317 brought to light some concurrency issues in the close method of Recyclers as we do not wait for threads running in the threadpool to be finished prior to the closing of the PageCacheRecycler and the Recyclers that are used internally. #41695 was opened to address the concurrent close issues but upon review, the closing of these classes is not really needed as the instances should be become available for garbage collection once there is no longer a reference to the closed node. Closes #41683 --- .../client/transport/TransportClient.java | 2 -- .../common/recycler/AbstractRecycler.java | 5 ----- .../common/recycler/ConcurrentDequeRecycler.java | 7 ------- .../elasticsearch/common/recycler/DequeRecycler.java | 10 ---------- .../elasticsearch/common/recycler/FilterRecycler.java | 5 ----- .../elasticsearch/common/recycler/NoneRecycler.java | 5 ----- .../org/elasticsearch/common/recycler/Recycler.java | 4 +--- .../org/elasticsearch/common/recycler/Recyclers.java | 7 ------- .../elasticsearch/common/util/PageCacheRecycler.java | 9 +-------- server/src/main/java/org/elasticsearch/node/Node.java | 2 -- .../common/recycler/AbstractRecyclerTestCase.java | 10 ---------- 11 files changed, 2 insertions(+), 64 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/client/transport/TransportClient.java b/server/src/main/java/org/elasticsearch/client/transport/TransportClient.java index b5720c023f095..4c2f4932de2f2 100644 --- a/server/src/main/java/org/elasticsearch/client/transport/TransportClient.java +++ b/server/src/main/java/org/elasticsearch/client/transport/TransportClient.java @@ -184,7 +184,6 @@ private static ClientTemplate buildTemplate(Settings providedSettings, Settings resourcesToClose.add(circuitBreakerService); PageCacheRecycler pageCacheRecycler = new PageCacheRecycler(settings); BigArrays bigArrays = new BigArrays(pageCacheRecycler, circuitBreakerService, CircuitBreaker.REQUEST); - resourcesToClose.add(pageCacheRecycler); modules.add(settingsModule); NetworkModule networkModule = new NetworkModule(settings, true, pluginsService.filterPlugins(NetworkPlugin.class), threadPool, bigArrays, pageCacheRecycler, circuitBreakerService, namedWriteableRegistry, xContentRegistry, networkService, null); @@ -376,7 +375,6 @@ public void close() { closeables.add(plugin); } closeables.add(() -> ThreadPool.terminate(injector.getInstance(ThreadPool.class), 10, TimeUnit.SECONDS)); - closeables.add(injector.getInstance(PageCacheRecycler.class)); IOUtils.closeWhileHandlingException(closeables); } diff --git a/server/src/main/java/org/elasticsearch/common/recycler/AbstractRecycler.java b/server/src/main/java/org/elasticsearch/common/recycler/AbstractRecycler.java index 05fa525972689..546d801d70b47 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/AbstractRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/AbstractRecycler.java @@ -28,9 +28,4 @@ protected AbstractRecycler(Recycler.C c) { this.c = c; } - @Override - public void close() { - // no-op by default - } - } diff --git a/server/src/main/java/org/elasticsearch/common/recycler/ConcurrentDequeRecycler.java b/server/src/main/java/org/elasticsearch/common/recycler/ConcurrentDequeRecycler.java index 04103c5e274d9..54374cc3bdebe 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/ConcurrentDequeRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/ConcurrentDequeRecycler.java @@ -37,13 +37,6 @@ public ConcurrentDequeRecycler(C c, int maxSize) { this.size = new AtomicInteger(); } - @Override - public void close() { - assert deque.size() == size.get(); - super.close(); - size.set(0); - } - @Override public V obtain() { final V v = super.obtain(); diff --git a/server/src/main/java/org/elasticsearch/common/recycler/DequeRecycler.java b/server/src/main/java/org/elasticsearch/common/recycler/DequeRecycler.java index a40befe9d8191..0f201133eceea 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/DequeRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/DequeRecycler.java @@ -36,16 +36,6 @@ public DequeRecycler(C c, Deque queue, int maxSize) { this.maxSize = maxSize; } - @Override - public void close() { - // call destroy() for every cached object - for (T t : deque) { - c.destroy(t); - } - // finally get rid of all references - deque.clear(); - } - @Override public V obtain() { final T v = deque.pollFirst(); diff --git a/server/src/main/java/org/elasticsearch/common/recycler/FilterRecycler.java b/server/src/main/java/org/elasticsearch/common/recycler/FilterRecycler.java index 426185173e581..5011402f6d97a 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/FilterRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/FilterRecycler.java @@ -34,9 +34,4 @@ public Recycler.V obtain() { return wrap(getDelegate().obtain()); } - @Override - public void close() { - getDelegate().close(); - } - } diff --git a/server/src/main/java/org/elasticsearch/common/recycler/NoneRecycler.java b/server/src/main/java/org/elasticsearch/common/recycler/NoneRecycler.java index 865182b88e104..102f1d424305a 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/NoneRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/NoneRecycler.java @@ -31,11 +31,6 @@ public V obtain() { return new NV<>(c.newInstance()); } - @Override - public void close() { - // no-op - } - public static class NV implements Recycler.V { T value; diff --git a/server/src/main/java/org/elasticsearch/common/recycler/Recycler.java b/server/src/main/java/org/elasticsearch/common/recycler/Recycler.java index 161e6463423f3..95a67fdf8e015 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/Recycler.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/Recycler.java @@ -25,7 +25,7 @@ * A recycled object, note, implementations should support calling obtain and then recycle * on different threads. */ -public interface Recycler extends Releasable { +public interface Recycler { interface Factory { Recycler build(); @@ -53,8 +53,6 @@ interface V extends Releasable { } - void close(); - V obtain(); } diff --git a/server/src/main/java/org/elasticsearch/common/recycler/Recyclers.java b/server/src/main/java/org/elasticsearch/common/recycler/Recyclers.java index 3ea9d17c25f19..5bfd3448e2336 100644 --- a/server/src/main/java/org/elasticsearch/common/recycler/Recyclers.java +++ b/server/src/main/java/org/elasticsearch/common/recycler/Recyclers.java @@ -145,13 +145,6 @@ protected Recycler getDelegate() { return recyclers[slot()]; } - @Override - public void close() { - for (Recycler recycler : recyclers) { - recycler.close(); - } - } - }; } diff --git a/server/src/main/java/org/elasticsearch/common/util/PageCacheRecycler.java b/server/src/main/java/org/elasticsearch/common/util/PageCacheRecycler.java index 4ca408e044170..40b9a8c7e9468 100644 --- a/server/src/main/java/org/elasticsearch/common/util/PageCacheRecycler.java +++ b/server/src/main/java/org/elasticsearch/common/util/PageCacheRecycler.java @@ -20,8 +20,6 @@ package org.elasticsearch.common.util; import org.apache.lucene.util.RamUsageEstimator; -import org.elasticsearch.common.lease.Releasable; -import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.recycler.AbstractRecyclerC; import org.elasticsearch.common.recycler.Recycler; import org.elasticsearch.common.settings.Setting; @@ -39,7 +37,7 @@ import static org.elasticsearch.common.recycler.Recyclers.none; /** A recycler of fixed-size pages. */ -public class PageCacheRecycler implements Releasable { +public class PageCacheRecycler { public static final Setting TYPE_SETTING = new Setting<>("cache.recycler.page.type", Type.CONCURRENT.name(), Type::parse, Property.NodeScope); @@ -73,11 +71,6 @@ public class PageCacheRecycler implements Releasable { NON_RECYCLING_INSTANCE = new PageCacheRecycler(Settings.builder().put(LIMIT_HEAP_SETTING.getKey(), "0%").build()); } - @Override - public void close() { - Releasables.close(true, bytePage, intPage, longPage, objectPage); - } - public PageCacheRecycler(Settings settings) { final Type type = TYPE_SETTING.get(settings); final long limit = LIMIT_HEAP_SETTING.get(settings).getBytes(); diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 4e8b81aea2e7b..3e3fef60febb1 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -378,7 +378,6 @@ protected Node( PageCacheRecycler pageCacheRecycler = createPageCacheRecycler(settings); BigArrays bigArrays = createBigArrays(pageCacheRecycler, circuitBreakerService); - resourcesToClose.add(pageCacheRecycler); modules.add(settingsModule); List namedWriteables = Stream.of( NetworkModule.getNamedWriteables().stream(), @@ -844,7 +843,6 @@ public synchronized void close() throws IOException { toClose.add(() -> stopWatch.stop()); toClose.add(injector.getInstance(NodeEnvironment.class)); - toClose.add(injector.getInstance(PageCacheRecycler.class)); if (logger.isTraceEnabled()) { logger.trace("Close times for each service:\n{}", stopWatch.prettyPrint()); diff --git a/server/src/test/java/org/elasticsearch/common/recycler/AbstractRecyclerTestCase.java b/server/src/test/java/org/elasticsearch/common/recycler/AbstractRecyclerTestCase.java index be7799fcd6c67..d2d12b32da4e1 100644 --- a/server/src/test/java/org/elasticsearch/common/recycler/AbstractRecyclerTestCase.java +++ b/server/src/test/java/org/elasticsearch/common/recycler/AbstractRecyclerTestCase.java @@ -99,7 +99,6 @@ public void testReuse() { assertNotSame(b1, b2); } o.close(); - r.close(); } public void testRecycle() { @@ -111,7 +110,6 @@ public void testRecycle() { o = r.obtain(); assertRecycled(o.v()); o.close(); - r.close(); } public void testDoubleRelease() { @@ -128,7 +126,6 @@ public void testDoubleRelease() { final Recycler.V v2 = r.obtain(); final Recycler.V v3 = r.obtain(); assertNotSame(v2.v(), v3.v()); - r.close(); } public void testDestroyWhenOverCapacity() { @@ -152,9 +149,6 @@ public void testDestroyWhenOverCapacity() { // release first ref, verify for destruction o.close(); assertDead(data); - - // close the rest - r.close(); } public void testClose() { @@ -171,10 +165,6 @@ public void testClose() { // verify that recycle() ran assertRecycled(data); - - // closing the recycler should mark recycled instances via destroy() - r.close(); - assertDead(data); } }