Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

5165-resource leaks in S3AccessIO calls #5305

Merged
merged 4 commits into from
Nov 14, 2018
Merged

5165-resource leaks in S3AccessIO calls #5305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd9ae67
Don't open input stream when not needed.
qqmyers Nov 12, 2018
bf4f94e
simplify
qqmyers Nov 13, 2018
40983e9
add @Override
qqmyers Nov 13, 2018
e07da5f
override getReadChannel as well
qqmyers Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/main/java/edu/harvard/iq/dataverse/api/Access.java
View file
Open in desktop
Expand Up @@ -651,15 +651,14 @@ public InputStream fileCardImage(@PathParam("fileId") Long fileId, @Context UriI
|| "application/zipped-shapefile".equalsIgnoreCase(df.getContentType())) {

thumbnailDataAccess = ImageThumbConverter.getImageThumbnailAsInputStream(dataAccess, 48);
if (thumbnailDataAccess != null && thumbnailDataAccess.getInputStream() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it applies here or not, but in past leak hunting removed several if (foo.getInputStream() != null) type statements, where the anonymous open statements appeared to be correlated with accumulating open file descriptors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pameyer - I think it should be OK in this case since the StorageIO class caches the InputStream - repeated calls to it don't create more streams as it would if you were directly opening a file stream, etc.
FWIW - In checking on this, I did find that that the code retrieved the channel, created from a stream, in order to create a new stream, rather than just retrieving the stream directly. Added a fix as a new commit.

return thumbnailDataAccess.getInputStream();
}
}
}
} catch (IOException ioEx) {
return null;
}

if (thumbnailDataAccess != null && thumbnailDataAccess.getInputStream() != null) {
return thumbnailDataAccess.getInputStream();
}

return null;
}
Expand Down Expand Up @@ -695,6 +694,9 @@ public InputStream dsCardImage(@PathParam("versionId") Long versionId, @Context
dataAccess.open();
thumbnailDataAccess = ImageThumbConverter.getImageThumbnailAsInputStream(dataAccess, 48);
}
if (thumbnailDataAccess != null && thumbnailDataAccess.getInputStream() != null) {
return thumbnailDataAccess.getInputStream();
}
} catch (IOException ioEx) {
thumbnailDataAccess = null;
}
Expand All @@ -711,10 +713,6 @@ public InputStream dsCardImage(@PathParam("versionId") Long versionId, @Context
}
}*/

if (thumbnailDataAccess != null && thumbnailDataAccess.getInputStream() != null) {
return thumbnailDataAccess.getInputStream();
}

}

return null;
Expand Down
11 changes: 5 additions & 6 deletions src/main/java/edu/harvard/iq/dataverse/dataaccess/ImageThumbConverter.java
View file
Open in desktop
Expand Up @@ -146,12 +146,11 @@ public static InputStreamIO getImageThumbnailAsInputStream(StorageIO<DataFile> s

try {
storageIO.open();
Channel cachedThumbnailChannel = storageIO.openAuxChannel(THUMBNAIL_SUFFIX + size);
if (cachedThumbnailChannel == null) {
logger.warning("Null channel for aux object " + THUMBNAIL_SUFFIX + size);
cachedThumbnailInputStream = storageIO.getAuxFileAsInputStream(THUMBNAIL_SUFFIX + size);
if (cachedThumbnailInputStream == null) {
logger.warning("Null stream for aux object " + THUMBNAIL_SUFFIX + size);
return null;
}
cachedThumbnailInputStream = Channels.newInputStream((ReadableByteChannel) cachedThumbnailChannel);
int cachedThumbnailSize = (int) storageIO.getAuxObjectSize(THUMBNAIL_SUFFIX + size);

InputStreamIO inputStreamIO = new InputStreamIO(cachedThumbnailInputStream, cachedThumbnailSize);
Expand Down Expand Up @@ -271,12 +270,12 @@ private static boolean generateImageThumbnail(StorageIO<DataFile> storageIO, int

try {
storageIO.open();
return generateImageThumbnailFromInputStream(storageIO, size, storageIO.getInputStream());
} catch (IOException ioex) {
logger.warning("caught IOException trying to open an input stream for " + storageIO.getDataFile().getStorageIdentifier() + ioex);
return false;
}

return generateImageThumbnailFromInputStream(storageIO, size, storageIO.getInputStream());

}

/*
Expand Down
51 changes: 39 additions & 12 deletions src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@
import com.amazonaws.services.s3.model.DeleteObjectsRequest;
import com.amazonaws.services.s3.model.DeleteObjectsRequest.KeyVersion;
import com.amazonaws.services.s3.model.GeneratePresignedUrlRequest;
import com.amazonaws.services.s3.model.GetObjectMetadataRequest;
import com.amazonaws.services.s3.model.GetObjectRequest;
import com.amazonaws.services.s3.model.ListObjectsRequest;
import com.amazonaws.services.s3.model.ObjectListing;
Expand All @@ -35,6 +36,7 @@
import java.net.URLEncoder;
import java.nio.channels.Channel;
import java.nio.channels.Channels;
import java.nio.channels.ReadableByteChannel;
import java.nio.channels.WritableByteChannel;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -157,22 +159,13 @@ public void open(DataAccessOption... options) throws IOException {

if (isReadAccess) {
key = getMainFileKey();
S3Object s3object = null;
ObjectMetadata objectMetadata = null;
try {
s3object = s3.getObject(new GetObjectRequest(bucketName, key));
objectMetadata = s3.getObjectMetadata(bucketName, key);
} catch (SdkClientException sce) {
throw new IOException("Cannot get S3 object " + key + " ("+sce.getMessage()+")");
}
InputStream in = s3object.getObjectContent();

if (in == null) {
throw new IOException("Cannot get InputStream for S3 Object" + key);
}

this.setInputStream(in);

setChannel(Channels.newChannel(in));
this.setSize(s3object.getObjectMetadata().getContentLength());
this.setSize(objectMetadata.getContentLength());

if (dataFile.getContentType() != null
&& dataFile.getContentType().equals("text/tab-separated-values")
Expand Down Expand Up @@ -215,6 +208,40 @@ public void open(DataAccessOption... options) throws IOException {
}
}

@Override
public InputStream getInputStream() throws IOException {
if(super.getInputStream()==null) {
try {
setInputStream(s3.getObject(new GetObjectRequest(bucketName, key)).getObjectContent());
} catch (SdkClientException sce) {
throw new IOException("Cannot get S3 object " + key + " ("+sce.getMessage()+")");
}
}

if (super.getInputStream() == null) {
throw new IOException("Cannot get InputStream for S3 Object" + key);
}

setChannel(Channels.newChannel(super.getInputStream()));

return super.getInputStream();
}

@Override
public Channel getChannel() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this @Override as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep...

if(super.getChannel()==null) {
getInputStream();
}
return channel;
}

@Override
public ReadableByteChannel getReadChannel() throws IOException {
//Make sure StorageIO.channel variable exists
getChannel();
return super.getReadChannel();
}

// StorageIO method for copying a local Path (for ex., a temp file), into this DataAccess location:
@Override
public void savePath(Path fileSystemPath) throws IOException {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java
View file
Open in desktop
Expand Up @@ -227,7 +227,7 @@ public boolean canWrite() {

// getters:

public Channel getChannel() {
public Channel getChannel() throws IOException {
return channel;
}

Expand Down Expand Up @@ -276,7 +276,7 @@ public long getSize() {
return size;
}

public InputStream getInputStream() {
public InputStream getInputStream() throws IOException {
return in;
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/edu/harvard/iq/dataverse/dataaccess/StorageIOTest.java
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@ public class StorageIOTest {
StorageIO<Dataset> instance = new FileAccessIO<>();

@Test
public void testGetChannel() throws FileNotFoundException {
public void testGetChannel() throws IOException {
assertEquals(null, instance.getChannel());
Channel c = new RandomAccessFile("src/main/java/Bundle.properties", "r").getChannel();
instance.setChannel(c);
Expand Down Expand Up @@ -104,7 +104,7 @@ public void testSize() {
}

@Test
public void testInputStream() {
public void testInputStream() throws IOException {
assertEquals(null, instance.getInputStream());
InputStream is = new ByteArrayInputStream("Test".getBytes());
instance.setInputStream(is);
Expand Down