Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

squid:S2095 - Resources should be closed #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

squid:S2095 - Resources should be closed #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hydra-main/src/main/java/com/addthis/hydra/job/store/JdbcDataStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ public Map<String, String> get(String[] paths) {
}
String command = sb.append(")").toString();
Map<String, String> rv = new HashMap<>();
try (Connection conn = cpds.getConnection()) {
PreparedStatement preparedStatement = conn.prepareStatement(command);
try (Connection conn = cpds.getConnection();
PreparedStatement preparedStatement = conn.prepareStatement(command)) {
// The first condition is that the child value is blankChildValue
preparedStatement.setString(1, blankChildValue);
int j = 2;
Expand Down
26 changes: 14 additions & 12 deletions hydra-main/src/main/java/com/addthis/hydra/job/store/MysqlDataStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,12 @@ public MysqlDataStore(String jdbcUrl, String dbName, String tableName, Propertie

@Override
protected void runSetupDatabaseCommand(final String dbName, final String jdbcUrl, final Properties properties) throws SQLException {
try (Connection connection = DriverManager.getConnection(jdbcUrl, properties)) {
String dbSetupCommand = String.format("CREATE DATABASE IF NOT EXISTS %s", dbName);
try (Connection connection = DriverManager.getConnection(jdbcUrl, properties);
PreparedStatement preparedStatement = connection.prepareStatement(dbSetupCommand)) {
// Create a connection that excludes the database from the jdbc url.
// This is necessary to create the database in the event that it does not exist.
String dbSetupCommand = String.format("CREATE DATABASE IF NOT EXISTS %s", dbName);
connection.prepareStatement(dbSetupCommand).execute();
preparedStatement.execute();
}
}

Expand All @@ -66,15 +67,16 @@ protected void runSetupDatabaseCommand(final String dbName, final String jdbcUrl
*/
@Override
protected void runSetupTableCommand() throws SQLException {
try (Connection connection = cpds.getConnection()) {
String tableSetupCommand = String.format("CREATE TABLE IF NOT EXISTS %s ( "
+ "%s INT NOT NULL AUTO_INCREMENT, " + // Auto-incrementing int id
"%s VARCHAR(%d) NOT NULL, %s MEDIUMBLOB, %s VARCHAR(%d), " + // VARCHAR path, BLOB value, VARCHAR child
"PRIMARY KEY (%s), UNIQUE KEY (%s,%s)) " + // Use id as primary key, enforce unique (path, child) combo
"ENGINE=%s", // Use specified table type (MyISAM works best in practice)
tableName, getIdKey(), getPathKey(), getMaxPathLength(), getValueKey(), getChildKey(),
getMaxPathLength(), getIdKey(), getPathKey(), getChildKey(), tableType);
connection.prepareStatement(tableSetupCommand).execute();
String tableSetupCommand = String.format("CREATE TABLE IF NOT EXISTS %s ( "
+ "%s INT NOT NULL AUTO_INCREMENT, " + // Auto-incrementing int id
"%s VARCHAR(%d) NOT NULL, %s MEDIUMBLOB, %s VARCHAR(%d), " + // VARCHAR path, BLOB value, VARCHAR child
"PRIMARY KEY (%s), UNIQUE KEY (%s,%s)) " + // Use id as primary key, enforce unique (path, child) combo
"ENGINE=%s", // Use specified table type (MyISAM works best in practice)
tableName, getIdKey(), getPathKey(), getMaxPathLength(), getValueKey(), getChildKey(),
getMaxPathLength(), getIdKey(), getPathKey(), getChildKey(), tableType);
try (Connection connection = cpds.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(tableSetupCommand)) {
preparedStatement.execute();
}
}

Expand Down
66 changes: 35 additions & 31 deletions hydra-main/src/main/java/com/addthis/hydra/job/store/PostgresDataStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ public PostgresDataStore(String jdbcUrl, String dbName, String tableName, Proper

@Override
protected void runSetupDatabaseCommand(String dbName, String jdbcUrl, Properties properties) throws SQLException {
try (Connection connection = DriverManager.getConnection(jdbcUrl, properties)) {
String dbSetupCommand = String.format("CREATE DATABASE %s", dbName);
try (Connection connection = DriverManager.getConnection(jdbcUrl, properties);
PreparedStatement preparedStatement = connection.prepareStatement(dbSetupCommand)) {
// Create a connection that excludes the database from the jdbc url.
// This is necessary to create the database in the event that it does not exist.
String dbSetupCommand = String.format("CREATE DATABASE %s", dbName);
connection.prepareStatement(dbSetupCommand).execute();
preparedStatement.execute();
} catch (final SQLException se) {
//the database may already exists
if (se.getMessage().endsWith("already exists")) {
Expand All @@ -60,34 +61,37 @@ protected void runSetupDatabaseCommand(String dbName, String jdbcUrl, Properties

@Override
protected void runSetupTableCommand() throws SQLException {
try (final Connection connection = cpds.getConnection()) {
final String tableSetupCommand = new StringBuffer("CREATE TABLE IF NOT EXISTS ").append(tableName).append(" ( ")
.append(getIdKey()).append(" SERIAL PRIMARY KEY, ") // Auto-incrementing int id
.append(getPathKey()).append(" VARCHAR(").append(getMaxPathLength()).append(") NOT NULL, ") // VARCHAR path
.append(getValueKey()).append(" VARCHAR, ")//VARCHAR value
.append(getChildKey()).append(" VARCHAR(").append(getMaxPathLength()).append("), ") // VARCHAR child
.append("CONSTRAINT parent_child UNIQUE (").append(getPathKey()).append(", ").append(getChildKey()).append(")) ") // enforce unique (path, child) combo
.toString();
connection.prepareStatement(tableSetupCommand).execute();
final String replaceFunctionSetupCommand = String.format(
"CREATE OR REPLACE FUNCTION replace_entry(%s VARCHAR, %s VARCHAR, %s VARCHAR) RETURNS void AS $$\n"
+ "BEGIN\n"
+ " IF EXISTS( SELECT * FROM %s WHERE %s = %s ) THEN\n"
+ " UPDATE %s\n"
+ " SET %s = %s, %s = %s WHERE %s = %s;\n"
+ " ELSE\n"
+ " INSERT INTO %s(%s, %s, %s) VALUES( %s, %s, %s );\n"
+ " END IF;\n"
+ "\n"
+ " RETURN;\n"
+ "END;\n"
+ "$$ LANGUAGE plpgsql;",
getPathKey() + "Var", getValueKey() + "Var", getChildKey() + "Var",
tableName, getPathKey(), getPathKey() + "Var",
tableName,
getValueKey(), getValueKey() + "Var", getChildKey(), getChildKey() + "Var" ,getPathKey(), getPathKey() + "Var",
tableName, getPathKey(), getValueKey(), getChildKey(), getPathKey() + "Var", getValueKey() + "Var", getChildKey() + "Var");
connection.prepareStatement(replaceFunctionSetupCommand).execute();
final String tableSetupCommand = new StringBuffer("CREATE TABLE IF NOT EXISTS ").append(tableName).append(" ( ")
.append(getIdKey()).append(" SERIAL PRIMARY KEY, ") // Auto-incrementing int id
.append(getPathKey()).append(" VARCHAR(").append(getMaxPathLength()).append(") NOT NULL, ") // VARCHAR path
.append(getValueKey()).append(" VARCHAR, ")//VARCHAR value
.append(getChildKey()).append(" VARCHAR(").append(getMaxPathLength()).append("), ") // VARCHAR child
.append("CONSTRAINT parent_child UNIQUE (").append(getPathKey()).append(", ").append(getChildKey()).append(")) ") // enforce unique (path, child) combo
.toString();
final String replaceFunctionSetupCommand = String.format(
"CREATE OR REPLACE FUNCTION replace_entry(%s VARCHAR, %s VARCHAR, %s VARCHAR) RETURNS void AS $$\n"
+ "BEGIN\n"
+ " IF EXISTS( SELECT * FROM %s WHERE %s = %s ) THEN\n"
+ " UPDATE %s\n"
+ " SET %s = %s, %s = %s WHERE %s = %s;\n"
+ " ELSE\n"
+ " INSERT INTO %s(%s, %s, %s) VALUES( %s, %s, %s );\n"
+ " END IF;\n"
+ "\n"
+ " RETURN;\n"
+ "END;\n"
+ "$$ LANGUAGE plpgsql;",
getPathKey() + "Var", getValueKey() + "Var", getChildKey() + "Var",
tableName, getPathKey(), getPathKey() + "Var",
tableName,
getValueKey(), getValueKey() + "Var", getChildKey(), getChildKey() + "Var" ,getPathKey(), getPathKey() + "Var",
tableName, getPathKey(), getValueKey(), getChildKey(), getPathKey() + "Var", getValueKey() + "Var", getChildKey() + "Var");
try (final Connection connection = cpds.getConnection();
PreparedStatement preparedStatement1 = connection.prepareStatement(tableSetupCommand);
PreparedStatement preparedStatement2 = connection.prepareStatement(replaceFunctionSetupCommand)) {
preparedStatement1.execute();
preparedStatement2.execute();

try {
final String createIndexCommand = String.format("CREATE INDEX ON %s (%s)", tableName, getPathKey());
connection.prepareStatement(createIndexCommand).execute();
Expand Down
11 changes: 9 additions & 2 deletions hydra-main/src/main/java/com/addthis/hydra/job/web/resources/AssetsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

import java.net.URI;

import org.slf4j.Logger;
Expand Down Expand Up @@ -71,9 +71,10 @@ public Response getIndex() {
public Response getAsset(@PathParam("target") String target) {
File file = new File(webDir, uriInfo.getPath());
if (file.exists() && file.isFile()) {
InputStream in = null;
try {
OutputStream out = response.getOutputStream();
InputStream in = new FileInputStream(file);
in = new FileInputStream(file);
byte[] buf = new byte[1024];
int read = 0;
while ((read = in.read(buf)) >= 0) {
Expand All @@ -86,6 +87,12 @@ public Response getAsset(@PathParam("target") String target) {
return Response.ok().build();
} catch (Exception ex) {
return Response.serverError().build();
} finally {
try {
in.close();
} catch (IOException e) {
log.error("Exception while closing resource", e);
}
}
} else {
if (log.isDebugEnabled()) log.debug("[http.unhandled] " + uriInfo.getPath());
Expand Down
7 changes: 4 additions & 3 deletions hydra-main/src/main/java/com/addthis/hydra/minion/LogUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ public final class LogUtils {
/** Streams task log files from newest to oldest. The returned Stream should be closed. */
public static Stream<Path> streamTaskLogsByName(JobTask task) throws IOException {
Path logDir = task.logDir.toPath();
return Files.list(logDir)
.filter(path -> Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS))
.sorted(Collections.reverseOrder());
try(Stream<Path> stream = Files.list(logDir)) {
return stream.filter(path -> Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS))
.sorted(Collections.reverseOrder());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're returning a Stream here -- it is kind of important that we don't close it prematurely. The javadoc also notes that the caller is responsible for closing it, and most importantly, no (non-fatal) exception can occur that leaves the file stream open. The IOException can only occur during the initial Files.list, and if it does, then it handles that cleanup internally.

}

public static Optional<Path> getNthNewestLog(JobTask task, int runsAgo, String suffix) throws IOException {
Expand Down
8 changes: 7 additions & 1 deletion hydra-main/src/main/java/com/addthis/hydra/minion/ProcessUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,22 @@ private static Integer findActiveProcessWithTokens(String[] requireTokens, Strin
command.append("| grep -v " + excludeToken);
}
command.append("| cut -c -5");
Scanner scanner = null;
try {
SimpleExec exec = new SimpleExec(new String[]{"/bin/sh", "-c", command.toString()}).join();
if (exec.exitCode() == 0) {
return new Scanner(exec.stdoutString()).nextInt();
scanner = new Scanner(exec.stdoutString());;
return scanner.nextInt();
} else {
return null;
}
} catch (Exception e) {
// No PID found
return null;
} finally {
if(scanner != null) {
scanner.close();
}
}

}
Expand Down
14 changes: 6 additions & 8 deletions hydra-main/src/main/java/com/addthis/hydra/query/MeshQueryMaster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@

import java.io.File;
import java.io.IOException;

import java.net.InetSocketAddress;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand All @@ -31,11 +29,11 @@
import java.util.TreeSet;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import com.addthis.basis.util.LessStreams;
import com.addthis.basis.util.LessFiles;
import com.addthis.basis.util.Parameter;

import com.addthis.hydra.data.query.Query;
import com.addthis.hydra.data.query.QueryException;
import com.addthis.hydra.query.aggregate.BalancedAllocator;
Expand All @@ -50,7 +48,6 @@
import com.addthis.hydra.query.tracker.TrackerHandler;
import com.addthis.meshy.MeshyServer;
import com.addthis.meshy.service.file.FileReference;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
Expand Down Expand Up @@ -246,10 +243,11 @@ protected void writeQuery(ChannelHandlerContext ctx, Query query, ChannelPromise
if (Strings.isNullOrEmpty(tasks)) {
return Collections.emptySet();
} else {
return LessStreams.stream(TASKS_SPLITTER.split(tasks))
.map(Ints::tryParse)
.filter(i -> i != null)
.collect(Collectors.toSet());
try(Stream<String> stream = LessStreams.stream(TASKS_SPLITTER.split(tasks))) {
return stream.map(Ints::tryParse)
.filter(i -> i != null)
.collect(Collectors.toSet());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike Files.list, most streams have a no-op close, and the API designers actually did not want obsessive resource protection where it was clearly unneeded. Consider, for example, that stream.map, and .filter also return Stream objects that are resources that could be closed.

Personally, I'm not a fan of the arbitrary "does close matter or not -- read the specifics each time and find out!" kind of API, but unless we want to wrap map and filter and so on, the line has to be drawn somewhere. A private method with a self-contained stream, backed by a simple String is one of the most provably harmless cases there is. I can't see the logic in guarding .stream in particular, or any of them for that matter.

}
}

Expand Down
29 changes: 15 additions & 14 deletions hydra-main/src/main/java/com/addthis/hydra/query/web/GoogleDriveAuthentication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,22 @@ private static void closeResource(@Nullable Closeable resource) {
* Send an HTML formatted error message.
*/
private static void sendErrorMessage(ChannelHandlerContext ctx, String message) throws IOException {
HttpResponse response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
response.headers().set(CONTENT_TYPE, "text/html; charset=utf-8");
StringBuilderWriter writer = new StringBuilderWriter(50);
writer.append("<html><head><title>Hydra Query Master</title></head><body>");
writer.append("<h3>");
writer.append(message);
writer.append("</h3></body></html>");
ByteBuf textResponse = ByteBufUtil.encodeString(ctx.alloc(),
try (StringBuilderWriter writer = new StringBuilderWriter(50)) {
HttpResponse response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
response.headers().set(CONTENT_TYPE, "text/html; charset=utf-8");
writer.append("<html><head><title>Hydra Query Master</title></head><body>");
writer.append("<h3>");
writer.append(message);
writer.append("</h3></body></html>");
ByteBuf textResponse = ByteBufUtil.encodeString(ctx.alloc(),
CharBuffer.wrap(writer.getBuilder()), CharsetUtil.UTF_8);
HttpContent content = new DefaultHttpContent(textResponse);
response.headers().set(HttpHeaders.Names.CONTENT_LENGTH, textResponse.readableBytes());
ctx.write(response);
ctx.write(content);
ChannelFuture lastContentFuture = ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT);
lastContentFuture.addListener(ChannelFutureListener.CLOSE);
HttpContent content = new DefaultHttpContent(textResponse);
response.headers().set(HttpHeaders.Names.CONTENT_LENGTH, textResponse.readableBytes());
ctx.write(response);
ctx.write(content);
ChannelFuture lastContentFuture = ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT);
lastContentFuture.addListener(ChannelFutureListener.CLOSE);
}
}

/**
Expand Down
8 changes: 4 additions & 4 deletions hydra-main/src/main/java/com/addthis/hydra/query/web/HttpQueryCallHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@

import java.net.InetSocketAddress;
import java.net.SocketAddress;

import java.util.Arrays;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import com.addthis.basis.kv.KVPairs;
import com.addthis.basis.util.Parameter;

import com.addthis.hydra.data.query.Query;
import com.addthis.hydra.data.query.source.ErrorHandlingQuerySource;
import com.addthis.hydra.data.query.source.QuerySource;
Expand All @@ -40,7 +39,6 @@
import io.netty.handler.codec.string.StringEncoder;
import io.netty.util.CharsetUtil;
import io.netty.util.concurrent.EventExecutor;

import static com.addthis.hydra.query.web.HttpUtils.sendError;

public final class HttpQueryCallHandler {
Expand All @@ -65,8 +63,10 @@ public static ChannelFuture handleQuery(ChannelHandler queryToQueryResultsEncode
if ((dir != null) && !job.endsWith(dir)) {
String[] jobs = job.split(",");
String[] dirs = dir.split(",");
job = Arrays.stream(jobs).flatMap(subJob -> Arrays.stream(dirs).map(subDir -> subJob + '/' + subDir))
try (Stream<String> stream = Arrays.stream(jobs)) {
stream.flatMap(subJob -> Arrays.stream(dirs).map(subDir -> subJob + '/' + subDir))
.collect(Collectors.joining(","));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same deal as before. Strings to strings to strings. There's no reason to fret over "leaking" the stream any more than there is to to be concerned about String[] jobs.

}
}
String path = kv.getValue("path", kv.getValue("q", ""));
Query query = new Query(job, new String[]{path}, new String[]{kv.getValue("ops"), kv.getValue("rops")});
Expand Down
39 changes: 18 additions & 21 deletions hydra-main/src/main/java/com/addthis/hydra/query/web/HttpStaticFileHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,35 +179,32 @@ public void channelRead0(

log.trace("sending {}", file);

FileChannel fileChannel;
FileChannel fileChannel = null;
try {
fileChannel = FileChannel.open(file, StandardOpenOption.READ);
} catch (IOException fnfe) {
sendError(ctx, NOT_FOUND);
return;
}
long fileLength = fileChannel.size();
long fileLength = fileChannel.size();

HttpResponse response = new DefaultHttpResponse(HTTP_1_1, OK);
setContentLength(response, fileLength);
setContentTypeHeader(response, file);
try {
HttpResponse response = new DefaultHttpResponse(HTTP_1_1, OK);
setContentLength(response, fileLength);
setContentTypeHeader(response, file);
setDateAndCacheHeaders(response, file);
if (isKeepAlive(request)) {
response.headers().set(CONNECTION, HttpHeaders.Values.KEEP_ALIVE);
}

// Write the initial line and the header.
ctx.write(response);

// Write the content.
ctx.write(new DefaultFileRegion(fileChannel, 0, fileLength));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Netty can be kind of a confusing beast, so I'm not surprised your tools got tripped up here. There's a couple issues, some rather significant:

  1. the fileChannel should not be closed right after ctx.write is called. That write is asynchronous, and it probably won't even try to write at all until that writeAndFlush is called later. In this case, DefaultFileRegion has some error recovery logic where it will construct a new RAF to get a channel from as long as it still has leases left, but that shouldn't be relied on and it breaks the lease paradigm as seen in

  2. As soon as we put the fileChannel into that wrapper and give it to ctx.write, it is no longer our business to close it. It will be closed in DefaultFileRegion's deallocate method when the latter runs out of leases (pretty much meaning in this case, when it has been fully written to the socket).

  3. As soon as ctx.write(response) was called, then queuing up the http response header et al, we should strongly prefer to avoid any code paths that could lead to eg. sendError(ctx, NOT_FOUND) being called. Most http clients become extremely upset when you send them conflicting http response codes.

  4. The separate catch blocks for the two sources of IOExceptions was not my original design (much of this class was appropriated from netty), but I think I would have gone with something not too dissimilar anyway. Of all the exception classes, IOException is the most common, and I would even say overused even if it isn't by its own choice (being checked and all, it gets forwarded everywhere). Although neither version provides as much information as it could, the base version from netty at least makes a compelling case that the author was sure the file vanished (for a moment, or for however long), and there's only two lines to double check that assertion. If I was returning to this class after a long time tracking down such an error, I highly doubt I would immediately suspect the now innocuously placed setDateAndCacheHeaders from being any more risky than its neighbors.

} catch (IOException ioex) {
fileChannel.close();
sendError(ctx, NOT_FOUND);
return;
} finally {
if(fileChannel != null) {
fileChannel.close();
}
}
if (isKeepAlive(request)) {
response.headers().set(CONNECTION, HttpHeaders.Values.KEEP_ALIVE);
}

// Write the initial line and the header.
ctx.write(response);

// Write the content.
ctx.write(new DefaultFileRegion(fileChannel, 0, fileLength));

// Write the end marker
ChannelFuture lastContentFuture = ctx.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT);

Expand Down