From 87ed84bbdc90d212ab03c5ff6c3862d253dd3e6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 25 May 2018 14:53:37 +0200 Subject: [PATCH] Replace some try-finally statements This change replaces some existing try-finally statements that close resources in their finally block with the slightly shorter and safer try-with-resources pattern. --- .../common/geo/parsers/GeoWKTParser.java | 10 +--- .../index/analysis/Analysis.java | 17 ++---- .../elasticsearch/index/engine/Engine.java | 4 -- .../index/get/ShardGetService.java | 4 +- .../index/termvectors/TermVectorsService.java | 13 ++-- .../elasticsearch/search/SearchService.java | 17 +++--- .../termvectors/MultiTermVectorsIT.java | 13 ++-- .../index/engine/InternalEngineTests.java | 60 +++++++++---------- .../index/shard/IndexShardTests.java | 20 ++++--- 9 files changed, 71 insertions(+), 87 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java index 20b159222d251..501f6ed59e687 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java @@ -18,12 +18,9 @@ */ package org.elasticsearch.common.geo.parsers; -import org.locationtech.jts.geom.Coordinate; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoShapeType; - -import java.io.StringReader; import org.elasticsearch.common.geo.builders.CoordinatesBuilder; import org.elasticsearch.common.geo.builders.EnvelopeBuilder; import org.elasticsearch.common.geo.builders.GeometryCollectionBuilder; @@ -37,9 +34,11 @@ import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.mapper.GeoShapeFieldMapper; +import org.locationtech.jts.geom.Coordinate; import java.io.IOException; import java.io.StreamTokenizer; +import java.io.StringReader; import java.util.List; /** @@ -77,8 +76,7 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoShapeType shapeType, final GeoShapeFieldMapper shapeMapper) throws IOException, ElasticsearchParseException { - StringReader reader = new StringReader(parser.text()); - try { + try (StringReader reader = new StringReader(parser.text())) { boolean ignoreZValue = (shapeMapper != null && shapeMapper.ignoreZValue().value() == true); // setup the tokenizer; configured to read words w/o numbers StreamTokenizer tokenizer = new StreamTokenizer(reader); @@ -95,8 +93,6 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue); checkEOF(tokenizer); return builder; - } finally { - reader.close(); } } diff --git a/server/src/main/java/org/elasticsearch/index/analysis/Analysis.java b/server/src/main/java/org/elasticsearch/index/analysis/Analysis.java index d736703f6418e..d56b8820e9b1c 100644 --- a/server/src/main/java/org/elasticsearch/index/analysis/Analysis.java +++ b/server/src/main/java/org/elasticsearch/index/analysis/Analysis.java @@ -234,8 +234,8 @@ public static List getWordList(Environment env, Settings settings, Strin final Path path = env.configFile().resolve(wordListPath); - try (BufferedReader reader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { - return loadWordList(reader, "#"); + try { + return loadWordList(path, "#"); } catch (CharacterCodingException ex) { String message = String.format(Locale.ROOT, "Unsupported character encoding detected while reading %s_path: %s - files must be UTF-8 encoded", @@ -247,15 +247,9 @@ public static List getWordList(Environment env, Settings settings, Strin } } - public static List loadWordList(Reader reader, String comment) throws IOException { + private static List loadWordList(Path path, String comment) throws IOException { final List result = new ArrayList<>(); - BufferedReader br = null; - try { - if (reader instanceof BufferedReader) { - br = (BufferedReader) reader; - } else { - br = new BufferedReader(reader); - } + try (BufferedReader br = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { String word; while ((word = br.readLine()) != null) { if (!Strings.hasText(word)) { @@ -265,9 +259,6 @@ public static List loadWordList(Reader reader, String comment) throws IO result.add(word.trim()); } } - } finally { - if (br != null) - br.close(); } return result; } diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index 7faaf51f4de6a..314eeffd7aa6a 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -1424,10 +1424,6 @@ public DocIdAndVersion docIdAndVersion() { @Override public void close() { - release(); - } - - public void release() { Releasables.close(searcher); } } diff --git a/server/src/main/java/org/elasticsearch/index/get/ShardGetService.java b/server/src/main/java/org/elasticsearch/index/get/ShardGetService.java index cb5ff580434f0..13f6e58cd79ec 100644 --- a/server/src/main/java/org/elasticsearch/index/get/ShardGetService.java +++ b/server/src/main/java/org/elasticsearch/index/get/ShardGetService.java @@ -159,7 +159,7 @@ private GetResult innerGet(String type, String id, String[] gFields, boolean rea get = indexShard.get(new Engine.Get(realtime, readFromTranslog, type, id, uidTerm) .version(version).versionType(versionType)); if (get.exists() == false) { - get.release(); + get.close(); } } } @@ -172,7 +172,7 @@ private GetResult innerGet(String type, String id, String[] gFields, boolean rea // break between having loaded it from translog (so we only have _source), and having a document to load return innerGetLoadFromStoredFields(type, id, gFields, fetchSourceContext, get, mapperService); } finally { - get.release(); + get.close(); } } diff --git a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java index 573e75d78060a..0148ab44d1b3e 100644 --- a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java +++ b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java @@ -85,8 +85,6 @@ static TermVectorsResponse getTermVectors(IndexShard indexShard, TermVectorsRequ termVectorsResponse.setExists(false); return termVectorsResponse; } - Engine.GetResult get = indexShard.get(new Engine.Get(request.realtime(), false, request.type(), request.id(), uidTerm) - .version(request.version()).versionType(request.versionType())); Fields termVectorsByField = null; AggregatedDfs dfs = null; @@ -97,8 +95,9 @@ static TermVectorsResponse getTermVectors(IndexShard indexShard, TermVectorsRequ handleFieldWildcards(indexShard, request); } - final Engine.Searcher searcher = indexShard.acquireSearcher("term_vector"); - try { + try (Engine.GetResult get = indexShard.get(new Engine.Get(request.realtime(), false, request.type(), request.id(), uidTerm) + .version(request.version()).versionType(request.versionType())); + Engine.Searcher searcher = indexShard.acquireSearcher("term_vector")) { Fields topLevelFields = MultiFields.getFields(get.searcher() != null ? get.searcher().reader() : searcher.reader()); DocIdAndVersion docIdAndVersion = get.docIdAndVersion(); /* from an artificial document */ @@ -143,14 +142,12 @@ else if (docIdAndVersion != null) { } } // write term vectors - termVectorsResponse.setFields(termVectorsByField, request.selectedFields(), request.getFlags(), topLevelFields, dfs, termVectorsFilter); + termVectorsResponse.setFields(termVectorsByField, request.selectedFields(), request.getFlags(), topLevelFields, dfs, + termVectorsFilter); } termVectorsResponse.setTookInMillis(TimeUnit.NANOSECONDS.toMillis(nanoTimeSupplier.getAsLong() - startTime)); } catch (Exception ex) { throw new ElasticsearchException("failed to execute term vector request", ex); - } finally { - searcher.close(); - get.release(); } return termVectorsResponse; } diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 8b4a3795c07dd..59af043e0cf7a 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -21,7 +21,6 @@ import org.apache.lucene.search.FieldDoc; import org.apache.lucene.search.TopDocs; -import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.action.ActionListener; @@ -39,6 +38,7 @@ import org.elasticsearch.common.util.concurrent.AbstractRunnable; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; import org.elasticsearch.common.util.concurrent.ConcurrentMapLong; +import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.IndexSettings; @@ -92,8 +92,8 @@ import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.search.suggest.Suggest; import org.elasticsearch.search.suggest.completion.CompletionSuggestion; -import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.Scheduler.Cancellable; +import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.ThreadPool.Names; import org.elasticsearch.transport.TransportRequest; @@ -646,20 +646,17 @@ private void freeAllContextForIndex(Index index) { public boolean freeContext(long id) { - final SearchContext context = removeContext(id); - if (context != null) { - assert context.refCount() > 0 : " refCount must be > 0: " + context.refCount(); - try { + try (SearchContext context = removeContext(id)) { + if (context != null) { + assert context.refCount() > 0 : " refCount must be > 0: " + context.refCount(); context.indexShard().getSearchOperationListener().onFreeContext(context); if (context.scrollContext() != null) { context.indexShard().getSearchOperationListener().onFreeScrollContext(context); } - } finally { - context.close(); + return true; } - return true; + return false; } - return false; } public void freeAllScrollContexts() { diff --git a/server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java b/server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java index 5ed4f3252d57c..10a4c9f3e1d7a 100644 --- a/server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java +++ b/server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java @@ -23,6 +23,7 @@ import org.apache.lucene.index.Fields; import org.apache.lucene.index.Terms; import org.apache.lucene.index.TermsEnum; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.admin.indices.alias.Alias; import org.elasticsearch.common.lucene.uid.Versions; import org.elasticsearch.common.settings.Settings; @@ -111,7 +112,8 @@ public void testMultiTermVectorsWithVersion() throws Exception { checkTermTexts(response.getResponses()[1].getResponse().getFields().terms("field"), new String[]{"value1"}); assertThat(response.getResponses()[2].getFailure(), notNullValue()); assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1")); - assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(VersionConflictEngineException.class)); + assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(ElasticsearchException.class)); + assertThat(response.getResponses()[2].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class)); //Version from Lucene index refresh(); @@ -132,7 +134,8 @@ public void testMultiTermVectorsWithVersion() throws Exception { checkTermTexts(response.getResponses()[1].getResponse().getFields().terms("field"), new String[]{"value1"}); assertThat(response.getResponses()[2].getFailure(), notNullValue()); assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1")); - assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(VersionConflictEngineException.class)); + assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(ElasticsearchException.class)); + assertThat(response.getResponses()[2].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class)); for (int i = 0; i < 3; i++) { @@ -155,7 +158,8 @@ public void testMultiTermVectorsWithVersion() throws Exception { assertThat(response.getResponses()[1].getFailure(), notNullValue()); assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2")); assertThat(response.getResponses()[1].getIndex(), equalTo("test")); - assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(VersionConflictEngineException.class)); + assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(ElasticsearchException.class)); + assertThat(response.getResponses()[1].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class)); assertThat(response.getResponses()[2].getId(), equalTo("2")); assertThat(response.getResponses()[2].getIndex(), equalTo("test")); assertThat(response.getResponses()[2].getFailure(), nullValue()); @@ -180,7 +184,8 @@ public void testMultiTermVectorsWithVersion() throws Exception { assertThat(response.getResponses()[1].getFailure(), notNullValue()); assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2")); assertThat(response.getResponses()[1].getIndex(), equalTo("test")); - assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(VersionConflictEngineException.class)); + assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(ElasticsearchException.class)); + assertThat(response.getResponses()[1].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class)); assertThat(response.getResponses()[2].getId(), equalTo("2")); assertThat(response.getResponses()[2].getIndex(), equalTo("test")); assertThat(response.getResponses()[2].getFailure(), nullValue()); diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 979c44dd5fc8d..26da424460ef2 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -21,6 +21,7 @@ import com.carrotsearch.hppc.cursors.ObjectObjectCursor; import com.carrotsearch.randomizedtesting.generators.RandomNumbers; + import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -793,7 +794,7 @@ public void testConcurrentGetAndFlush() throws Exception { while (flushFinished.get() == false) { Engine.GetResult previousGetResult = latestGetResult.get(); if (previousGetResult != null) { - previousGetResult.release(); + previousGetResult.close(); } latestGetResult.set(engine.get(newGet(true, doc), searcherFactory)); if (latestGetResult.get().exists() == false) { @@ -807,7 +808,7 @@ public void testConcurrentGetAndFlush() throws Exception { flushFinished.set(true); getThread.join(); assertTrue(latestGetResult.get().exists()); - latestGetResult.get().release(); + latestGetResult.get().close(); } public void testSimpleOperations() throws Exception { @@ -830,21 +831,20 @@ public void testSimpleOperations() throws Exception { searchResult.close(); // but, not there non realtime - Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(false)); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(false)); + } // but, we can still get it (in realtime) - getResult = engine.get(newGet(true, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(true)); - assertThat(getResult.docIdAndVersion(), notNullValue()); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(true)); + assertThat(getResult.docIdAndVersion(), notNullValue()); + } // but not real time is not yet visible - getResult = engine.get(newGet(false, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(false)); - getResult.release(); - + try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(false)); + } // refresh and it should be there engine.refresh("test"); @@ -856,10 +856,10 @@ public void testSimpleOperations() throws Exception { searchResult.close(); // also in non realtime - getResult = engine.get(newGet(false, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(true)); - assertThat(getResult.docIdAndVersion(), notNullValue()); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(true)); + assertThat(getResult.docIdAndVersion(), notNullValue()); + } // now do an update document = testDocument(); @@ -876,10 +876,10 @@ public void testSimpleOperations() throws Exception { searchResult.close(); // but, we can still get it (in realtime) - getResult = engine.get(newGet(true, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(true)); - assertThat(getResult.docIdAndVersion(), notNullValue()); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(true)); + assertThat(getResult.docIdAndVersion(), notNullValue()); + } // refresh and it should be updated engine.refresh("test"); @@ -901,9 +901,9 @@ public void testSimpleOperations() throws Exception { searchResult.close(); // but, get should not see it (in realtime) - getResult = engine.get(newGet(true, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(false)); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(false)); + } // refresh and it should be deleted engine.refresh("test"); @@ -941,10 +941,10 @@ public void testSimpleOperations() throws Exception { engine.flush(); // and, verify get (in real time) - getResult = engine.get(newGet(true, doc), searcherFactory); - assertThat(getResult.exists(), equalTo(true)); - assertThat(getResult.docIdAndVersion(), notNullValue()); - getResult.release(); + try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) { + assertThat(getResult.exists(), equalTo(true)); + assertThat(getResult.docIdAndVersion(), notNullValue()); + } // make sure we can still work with the engine // now do an update @@ -4156,7 +4156,7 @@ public void testSeqNoGenerator() throws IOException { new Term("_id", parsedDocument.id()), parsedDocument, SequenceNumbers.UNASSIGNED_SEQ_NO, - (long) randomIntBetween(1, 8), + randomIntBetween(1, 8), Versions.MATCH_ANY, VersionType.INTERNAL, Engine.Operation.Origin.PRIMARY, @@ -4172,7 +4172,7 @@ public void testSeqNoGenerator() throws IOException { id, new Term("_id", parsedDocument.id()), SequenceNumbers.UNASSIGNED_SEQ_NO, - (long) randomIntBetween(1, 8), + randomIntBetween(1, 8), Versions.MATCH_ANY, VersionType.INTERNAL, Engine.Operation.Origin.PRIMARY, diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java index 31e51ed43d40e..027b595ee761f 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -1861,10 +1861,11 @@ public void testSearcherWrapperIsUsed() throws IOException { indexDoc(shard, "_doc", "1", "{\"foobar\" : \"bar\"}"); shard.refresh("test"); - Engine.GetResult getResult = shard.get(new Engine.Get(false, false, "test", "1", new Term(IdFieldMapper.NAME, Uid.encodeId("1")))); - assertTrue(getResult.exists()); - assertNotNull(getResult.searcher()); - getResult.release(); + try (Engine.GetResult getResult = shard + .get(new Engine.Get(false, false, "test", "1", new Term(IdFieldMapper.NAME, Uid.encodeId("1"))))) { + assertTrue(getResult.exists()); + assertNotNull(getResult.searcher()); + } try (Engine.Searcher searcher = shard.acquireSearcher("test")) { TopDocs search = searcher.searcher().search(new TermQuery(new Term("foo", "bar")), 10); assertEquals(search.totalHits, 1); @@ -1895,11 +1896,12 @@ public IndexSearcher wrap(IndexSearcher searcher) throws EngineException { search = searcher.searcher().search(new TermQuery(new Term("foobar", "bar")), 10); assertEquals(search.totalHits, 1); } - getResult = newShard.get(new Engine.Get(false, false, "test", "1", new Term(IdFieldMapper.NAME, Uid.encodeId("1")))); - assertTrue(getResult.exists()); - assertNotNull(getResult.searcher()); // make sure get uses the wrapped reader - assertTrue(getResult.searcher().reader() instanceof FieldMaskingReader); - getResult.release(); + try (Engine.GetResult getResult = newShard + .get(new Engine.Get(false, false, "test", "1", new Term(IdFieldMapper.NAME, Uid.encodeId("1"))))) { + assertTrue(getResult.exists()); + assertNotNull(getResult.searcher()); // make sure get uses the wrapped reader + assertTrue(getResult.searcher().reader() instanceof FieldMaskingReader); + } closeShards(newShard); }