Use of RequestPart with String leads to a file descriptor leak

[samudra77]

StringHttpMessageConvertor is reading the InputStream but it's not closing it.
This keeps the file handle open and leaks file handles.
I am using @RequestPart in rest controller with string parameter.
the request is a multipart form data. Other Jackson converters are closing the streams.
The stream is pointing to a temporary file created by servlet api on tomcat.

[symphony-youri]

I'm observing this issue too with 2.2.10.RELEASE and more recent versions (up to 2.6.1)

[rstoyanchev]

The behavior goes back to 6661788, and looking at the description in #14728 and #14869, it was related to a server writing with multipart data. However, it was also applied on the reading side too, and it's not clear why, just from reading the linked issues.

One option would be to re-introduce FileCopyUtils on the reading side. Another, to close the InputStream on the server side after reading the body, e.g. in AbstractMessageConverterMethodArgumentResolver. There is a PR along those lines #25897.

[symphony-youri]

As a workaround, moving from @RequestPart to @RequestParam for strings parameters fixes the issue (as the converter won't be used).

[samudra77]

Yes I have changed the code to use request param for string arguments. But wondering if it's also server specific? Noticed this happening with Tomcat after recent patches applied on but couldn't figure out who is the culprit and should be responsible for closing the stream. Someone has to close it. Application code unfortunately has no handle to it.

[symphony-youri]

Interesting, I'm wondering why this issue has been unnoticed in our application. And it has been detected by one user after we (force) upgraded the dependency to tomcat core to 9.0.54 (was 9.0.38 before).

[symphony-youri]

Actually changing to @RequestParam broke some of our API clients that were sending a part with a filename set even though it was a string content. So we had to revert this change.

We are trying another workaround based on a RequestBodyAdvice:

@ControllerAdvice
@Component
public class CloseStringRequestPartAdvice extends RequestBodyAdviceAdapter {
  @Override
  public boolean supports(MethodParameter methodParameter, Type targetType, Class<? extends HttpMessageConverter<?>> converterType) {
    // being extra careful to only apply it for @RequestPart String parameters
    return targetType != null
        && String.class.getTypeName().equals(targetType.getTypeName())
        && converterType.equals(StringHttpMessageConverter.class)
        && methodParameter.getParameterAnnotations() != null
        && methodParameter.getParameterAnnotations().length >= 1
        && RequestPart.class.equals(methodParameter.getParameterAnnotations()[0].annotationType());
  }

  @Override
  public Object afterBodyRead(Object body, HttpInputMessage inputMessage, MethodParameter parameter, Type targetType, Class<? extends HttpMessageConverter<?>> converterType) {
    // extra check, to really be sure we try to close it when needed
    if (supports(parameter, targetType, converterType)) {
      try {
        inputMessage.getBody().close();
      } catch (Exception e) {
        // we rather silently fail than prevent the request from working and eventually leaking a file descriptor
        LOG.debug("Failed to close form data string", e);
      }
    }
    return body;
  }
}

There are extra (and specific to our use case probably) checks done to avoid closing streams inadvertently.

[rstoyanchev]

@samudra77 and @symphony-youri, can you confirm what kind of part this is for, is it a file or just plain text?

I see a potentially related issue in Tomcat https://bz.apache.org/bugzilla/show_bug.cgi?id=65710 that suggests a JVM issue, which can be confirmed by trying with Java 11. There was also a fix implemented in Tomcat 9.0.56 to make up for the issue, so you could try upgrading to 9.0.56 too and see if it makes a difference?

[symphony-youri]

It was for plain text (that ends up serialized as a temp file).
I'll try to update to see if it makes a difference. Thanks for spotting the Tomcat issue.

[samudra77]

We are also passing string data which is leaking. File data requests are not leaking. We are on Tomcat 8.5 and I have read the Tomcat issue and looked at the Tomcat class which doesn't seem to have the problematic line mentioned in the Tomcat issue thread

new FileInputStream(dfos.getFile());
to:
Files.newInputStream(dfos.getFile().toPath());

But regardless of that. I am thinking if StringHttpMessageConverter is doing the right thing or not. Should it close the stream it's reading after converting to string.

[symphony-youri]

I tried with Tomcat 9.0.56 (by upgrading to Spring Boot 2.6.2) and the issue is still there.
I also tried with Java 11 and it still happens.

[rstoyanchev]

Thanks for trying. I think the auto-cleanup happens with garbage collection through Tomcat's DiskFileItem#finalize method, and/or similar mechanisms in the file I/O of the JDK are also potentially at play. Keep in mind the reporter on the Tomcat issue is trying with a while loop sending requests, so it's possible that it does make a difference still depending on how you're checking for the leak.

That said we don't want to rely on any of these implicit mechanism and we will ensure that the resolver for @RequestPart closes the InputStream as it should.