ResponseEntity<Resource> is not closed when an exception occurs

[Sycamore-M]

Environment: SpringBoot 2.7.10, SpringWeb 5.3.26, Java 11

There is a file download api which uses Amazon S3, and the return result is a ResponseEntity<Resource>

@GetMapping("/download/{id}")
public ResponseEntity<Resource> download(@PathVariable("id") String id) {
    S3Object s3Object = amazonS3.getObject(bucket, id);
    ObjectMetadata metadata = s3Object.getObjectMetadata();
    Resource resource = new InputStreamResource(s3Object.getObjectContent());
    return ResponseEntity.ok()
            .header(HttpHeaders.CONTENT_TYPE, metadata.getContentType())
            .header(HttpHeaders.CONTENT_LENGTH, String.valueOf(metadata.getContentLength()))
            .body(resource);
}

It works fine until invalid content types appear, such as "excel". The InputStream of the S3Object is not closed because SpringWeb failed to parse the content type. It leads to memory leaks and connection pool leaks.

I can not do anything in this situation because I am not sure if the return will result in any exceptions. So I have to use try-with-resources instead:

@GetMapping("/download/{id}")
public void download(@PathVariable("id") String id, HttpServletResponse response) throws IOException {
    S3Object s3Object = amazonS3.getObject(bucket, id);
    try (InputStream inputStream = s3Object.getObjectContent()) {
        ObjectMetadata metadata = s3Object.getObjectMetadata();
        response.setHeader(HttpHeaders.CONTENT_TYPE, metadata.getContentType());
        response.setHeader(HttpHeaders.CONTENT_LENGTH, String.valueOf(metadata.getContentLength()));
        inputStream.transferTo(response.getOutputStream());
    }
}

In my opinion, the Spring framework should be responsible for closing the input stream that I return, no mater what exception occurs.

If my viewpoint is incorrect, please tell me the standard implementation for file downloads.

Thanks

[jhoeller]

The root of the problem is the use of InputStreamResource with a pre-obtained InputStream here. Once you pass that step, there is always a chance of failing before the InputStream is actually read and closed. Your workaround seems fine for the time being there, addressing that concern through wrapping the entire InputStream read step with a try-with-resources block.

If you'd like to keep using Spring's Resource abstraction within ResponseEntity, you'd have to pass in a Resource with on-demand InputStream retrieval - along the following lines:

	Resource resource = new AbstractResource() {
		@Override
		public InputStream getInputStream() throws IOException {
			return s3Object.getObjectContent();
		}
		@Override
		public long contentLength() {
			return s3Metadata.getContentLength();
		}
		@Override
		public String getDescription() {
			return "S3 resource: " + s3Object;
		}
	};

The contentLength() part is optional for implicit exposure of the HTTP Content-Length header.

That said, even the above might be insufficient for S3 since apparently the stream is opened when the S3Object instance is obtained, with getObjectContent() just accessing it. The direct streaming in your try-with-resources block might be the only reliable choice for the S3 API then, guaranteeing an InputStream.close() call for any intermediate failure.

In general, file downloads via Resource should always use the most specific Resource variant with on-demand stream access: either from a ResourceLoader (typically classpath or file system based) or through specific construction of a UrlResource or the like.

InputStreamResource is a bit of a last resort and an easy trap to fall into, unfortunately. We could make it less of a trap by accepting a Supplier<InputStream> in the constructor, even if this might not be sufficient in an S3 scenario either. In addition, we should provide a guideline for ResponseEntity<Resource> in the documentation.

[Sycamore-M]

Thank you for your answer, which gave me a more accurate understanding of ResponseEntity<Resource>.

[jhoeller]

In addition to specific notes in the reference documentation, I'm also taking the opportunity to add an InputStreamResource(InputStreamSource) constructor where the argument can be provided as a lambda expression that lazily retrieves the actual InputStream, with corresponding hints in the javadoc that this is preferable for reliable closing.