21

One of our application leaks file handles and we have not yet found the cause for this.

In the code I can see several functions similar to this:

public ResponseEntity<InputStreamResource> getFoo( ... ) {
    InputStream content = getContent(...)
    InputStreamResource isr = new InputStreamResource(content);
    return ResponseEntity.status(HttpServletResponse.SC_OK).body(isr);
}

(if checks and try / catch removed for brevity)

I am sure this section causes the problem because when I loadtest this specific code with JMeter I can see that getContent() fails in this stage:

is = Files.newInputStream(f.toPath());

Normally I would close the InputStream but because this short and simply code I can't close the stream before return or the call of body.

When I run lsof (the code runs on Linux) I can see that thousands of files are open in read mode. So I am sure this problem is caused by the stream not getting closed.

Is there a best practice code I should trade in ?

Marged
  • 9,123
  • 9
  • 45
  • 87

4 Answers4

33

you can try to use StreamingResponseBody

StreamingResponseBody

A controller method return value type for asynchronous request processing where the application can write directly to the response OutputStream without holding up the Servlet container thread.

Because you are working on a separate thread, writing directly to the response, your problem to call close() before return is solved.

probably you can start by the following example

public ResponseEntity<StreamingResponseBody> export(...) throws FileNotFoundException {
    //...

    InputStream inputStream = new FileInputStream(new File("/path/to/example/file"));


    StreamingResponseBody responseBody = outputStream -> {

        int numberOfBytesToWrite;
        byte[] data = new byte[1024];
        while ((numberOfBytesToWrite = inputStream.read(data, 0, data.length)) != -1) {
            System.out.println("Writing some bytes..");
            outputStream.write(data, 0, numberOfBytesToWrite);
        }

        inputStream.close();
    };

    return ResponseEntity.ok()
            .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=generic_file_name.bin")
            .contentType(MediaType.APPLICATION_OCTET_STREAM)
            .body(responseBody);
}

You can also try to use Files (since java 7)

so you don't have to manage InputStream

    File file = new File("/path/to/example/file");

    StreamingResponseBody responseBody = outputStream -> {
        Files.copy(file.toPath(), outputStream);
    };

As @Stackee007 described in comment, under heavy load in production environment it's a good practice also to define a @Configuration class for a TaskExecutor to tune parameters and manage Async processes.

@Configuration
@EnableAsync
@EnableScheduling
public class AsyncConfiguration implements AsyncConfigurer {

    private final Logger log = LoggerFactory.getLogger(AsyncConfiguration.class);

    private final TaskExecutionProperties taskExecutionProperties;

    public AsyncConfiguration(TaskExecutionProperties taskExecutionProperties) {
        this.taskExecutionProperties = taskExecutionProperties;
    }

    //  ---------------> Tune parameters here
    @Override
    @Bean(name = "taskExecutor")
    public Executor getAsyncExecutor() {
        log.debug("Creating Async Task Executor");
        ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
        executor.setCorePoolSize(taskExecutionProperties.getPool().getCoreSize());
        executor.setMaxPoolSize(taskExecutionProperties.getPool().getMaxSize());
        executor.setQueueCapacity(taskExecutionProperties.getPool().getQueueCapacity());
        executor.setThreadNamePrefix(taskExecutionProperties.getThreadNamePrefix());
        return executor;
    }
    
    //  ---------------> Use this task executor also for async rest methods
    @Bean
    protected WebMvcConfigurer webMvcConfigurer() {
        return new WebMvcConfigurer() {
            @Override
            public void configureAsyncSupport(AsyncSupportConfigurer configurer) {
                configurer.setTaskExecutor(getTaskExecutor());
            }
        };
    }

    @Bean
    protected ConcurrentTaskExecutor getTaskExecutor() {
        return new ConcurrentTaskExecutor(this.getAsyncExecutor());
    }

    @Override
    public AsyncUncaughtExceptionHandler getAsyncUncaughtExceptionHandler() {
        return new SimpleAsyncUncaughtExceptionHandler();
    }
}

How to test with mockMvc

You can simply follow this sample code in your integration test as:

    .andExpect(request().asyncStarted())
    .andDo(MvcResult::getAsyncResult)
    .andExpect(status().isOk()).getResponse().getContentAsByteArray();

Content type of ResponseEntity<StreamingResponseBody> is a MediaType.APPLICATION_OCTET_STREAM in this example and you can get byte[] (.getContentAsByteArray()) but you can get String/Json/plaintext of everything depending of your body response content type.

ValerioMC
  • 2,261
  • 9
  • 16
  • This looks promising. Do you want to beef this up a little bit by adding more information ? – Marged Sep 27 '18 at 04:03
  • Do heed the note on using StreamingBodyResponse "Note: when using this option it is highly recommended to configure explicitly the TaskExecutor used in Spring MVC for executing asynchronous requests. Both the MVC Java config and the MVC namespaces provide options to configure asynchronous handling. If not using those, an application can set the taskExecutor property of RequestMappingHandlerAdapter." – Stackee007 Sep 27 '18 at 16:23
  • added implementation of a TaskExecutor configuration – ValerioMC Jul 10 '19 at 09:11
  • How to test it with `mockMvc` - add `.andExpect(request().asyncStarted()) .andDo(MvcResult::getAsyncResult)` to your integration tests. [source](http://digitallumberjack.me/post/test-streaming-springboot/) – user11153 Jan 28 '20 at 12:56
  • i've written a little example with mockMvc integration test – ValerioMC Jul 07 '20 at 12:36
  • Is it possible to use StreamingBodyResponse with Jdk 1.6, i have legacy project that needs to use asynch response streaming. – fiddle Mar 08 '21 at 08:59
5

Assuming you are using Spring, your method could return a Resource and let Spring handle the rest (including closing underlying stream). There are few implementations of Resource are available within Spring API or else you need to implement your own. In the end, your method would become simple and would like something like below

public ResponseEntity<Resource> getFo0(...) {
    return new InputStreamResource(<Your input stream>);
}
Stackee007
  • 3,038
  • 1
  • 23
  • 39
3

You could refactor all your controller methods that read local files and set their contents as the body of the HTTP response:

Instead of using the ResponseEntity approach you inject the underlying HttpServletResponse and copy the bytes of the inputstream returned from your getContent(...) method to the outputstream of the HttpServletResponse, e.g. by using IO-related utility methods of Apache CommonsIO or Google Guava library. In any case make sure you close the inputstream! The code below does this implicitly by the use of a 'try-with-resources'-statement that closes the declared inputstream at the end of the statement.

@RequestMapping(value="/foo", method=RequestMethod.GET)
public void getFoo(HttpServletResponse response) {
    // use Java7+ try-with-resources
    try (InputStream content = getContent(...)) {

        // if needed set content type and attachment header
        response.addHeader("Content-disposition", "attachment;filename=foo.txt");
        response.setContentType("txt/plain");

        // copy content stream to the HttpServletResponse's output stream
        IOUtils.copy(myStream, response.getOutputStream());

        response.flushBuffer();
    }
}

reference:

https://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html https://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/ByteStreams.html https://commons.apache.org/proper/commons-io/javadocs/api-release/index.html

(especially have a look at methods public static int copy(InputStream input, OutputStream output) throws IOException and public static int copyLarge(InputStream input, OutputStream output) throws IOException of class org.apache.commons.io.IOUtils)

  • I like @Marged approach (in case it has no caveats I'm not aware of) a little more for its conciseness and as it's possibly more consequent to work with `ReponseEntity` in _all_ endpoints. Nevertheless we've been using Tommy's approach in several applications and it works flawlessy. – Jan B. Sep 26 '18 at 22:29
  • 1
    Spring's [FileCopyUtils](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/util/FileCopyUtils.html) can be used as well - it closes the stream for you so one thing less to remember than IO-related utility methods of Apache CommonsIO or Google Guava library. – Krzysztof Skrzynecki Aug 11 '19 at 10:07
  • Am I right that the main advantage of `StreamingResponseBody` over using `HttpServletResponse#getOutputStream` is that you can start `TaskExecutor`, so writing data to stream won't [hold up the Servlet container thread busy](https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/web/servlet/mvc/method/annotation/StreamingResponseBody.html)? I'm just trying to understand what is the difference between those two. – Krzysztof Skrzynecki Aug 11 '19 at 10:10
2

Because this InputStream is basically coming from a simple file, a good replacement is this code:

FileSystemResource fsr = new FileSystemResource(fileName);
return ResponseEntity.status(HttpServletResponse.SC_OK).body(fsr);

FileSystemResource can take a java.util.File, a java.nio.file.Path or even a String pointing to the relevant file.

Marged
  • 9,123
  • 9
  • 45
  • 87