Skip to content

Commit

Permalink
Replace several try-finally statements (#30880)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Christoph Büscher authored May 29, 2018
1 parent 89869a2 commit c137ad0
Show file tree
Hide file tree
Showing 9 changed files with 71 additions and 87 deletions.
10 changes: 3 additions & 7 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -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);
Expand All @@ -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();
}
}

Expand Down
17 changes: 4 additions & 13 deletions server/src/main/java/org/elasticsearch/index/analysis/Analysis.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ public static List<String> 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",
Expand All @@ -247,15 +247,9 @@ public static List<String> getWordList(Environment env, Settings settings, Strin
}
}

public static List<String> loadWordList(Reader reader, String comment) throws IOException {
private static List<String> loadWordList(Path path, String comment) throws IOException {
final List<String> 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)) {
Expand All @@ -265,9 +259,6 @@ public static List<String> loadWordList(Reader reader, String comment) throws IO
result.add(word.trim());
}
}
} finally {
if (br != null)
br.close();
}
return result;
}
Expand Down
4 changes: 0 additions & 4 deletions server/src/main/java/org/elasticsearch/index/engine/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1424,10 +1424,6 @@ public DocIdAndVersion docIdAndVersion() {

@Override
public void close() {
release();
}

public void release() {
Releasables.close(searcher);
}
}
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/get/ShardGetService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Expand All @@ -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();
}
}

Expand Down
13 changes: 5 additions & 8 deletions server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 */
Expand Down Expand Up @@ -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;
}
Expand Down
17 changes: 7 additions & 10 deletions server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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() {
Expand Down
13 changes: 9 additions & 4 deletions server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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++) {
Expand All @@ -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());
Expand All @@ -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());
Expand Down
60 changes: 30 additions & 30 deletions server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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");
Expand All @@ -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();
Expand All @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Loading

0 comments on commit c137ad0

Please sign in to comment.