Affects: 5.1.5
I return a stream with BodyBuilder.body(new InputStreamResource(stream)) from a @Controller:
@SpringBootApplication
@Controller
public class Issue22626 {
@Autowired(required = false)
private Supplier<InputStream> source = () -> new NullInputStream(10000);
@GetMapping("/")
public ResponseEntity<InputStreamResource> download() {
var resource = new InputStreamResource(source.get());
return ok().eTag("Issue22626").lastModified(22626).body(resource);
}
public static void main(String[] args) {
run(Issue22626.class, args);
}
}
That stream is a proxy which helps me to detect resource leaks. The proxy contains the stack trace of the last read operation and a scheduled task to close the resource and log an error after 10 minutes of inactivity.
In the region 5 out of 10000 streams, I see a resource leak. That is, the log shows the "Returning resource" message, then shows that the thread is already serving others requests and then after 10 minutes I see a message from my stream proxy that the resource is leaking. There was no read operation at all called on that stream. Which means the framework left the resource untouched and never reaches ResourceHttpMessageConverter.writeContent() (which would then close the stream). I tried to dig into the framework, but it is for me not possible to understand what is going on at all.
~Note, this is an production issue which happens in the region 5 out of 10000, I cannot provide something to reproduce this memory leak. I will however try my best to give some more follow up information.~ I will provide a minimal application to reproduce the issue.
All I can see is that the framework assumes that ResourceHttpMessageConverter.writeContent() would close the resource. I think that is too late. Obviously many things could happen before reaching that place (and something silently does… ~my suspicion goes into a unfortunately timed client sided connection closing~) which leaves the resource unclosed. Could you please revise the framework code in regard of serving a Controller which returns an InputStream and make sure that the very first thing into which this Controller returns wraps the stream into a try-with-resource block?
Comment From: mixcloud-downloader
I got more context. The response code is 304, which I didn't set (I respond with 200). Help me out, this is framework behavior isn't it? The request contains if-modified-since and if-none-match headers. I do add and ETag and Last-Modified header in my response.
I think with this I could create a minimal application to reproduce the issue. Wait for it…
Comment From: mixcloud-downloader
Here's a minimal test case to demonstrate the issue: https://github.com/spring-projects/spring-framework-issues/pull/187
Comment From: rstoyanchev
So with a 304 we don't need to write the content to the response (that's the point of notModified) and don't need to read the Resource and therefore don't need to close it.
Instead of creating the InputStream eagerly, I suggest returning a Resource that defers the creation of the InputStream until getInputStream is called. If you look in InputStreamResource it's very small and easy to create a deferred variant of that.
Comment From: mixcloud-downloader
Wait, from a user point of view I don't even notice that. As a user I just write a controller which returns a ResponseEntity<InputStreamResource>. I was totally unaware of the framework magic which happens afterwards and leads to the memory leak. If you decide that's an OK behavior for the framework, don't you at least think it need some bit of documentation that InputStreamResource is broken by design?
Edit: Also just not opening the resource in the controller because the framework decides somewhere later not to read it is not trivial at all. In my case I need to open the resource beforehand to know which Headers I would want to send. From that moment on the resource is open and now I give up control to the framework. Would the framework please be so kind and if it decides not to read the resource please just close it. It is a very unintuitive design that the framework sometimes closes the resource and sometimes not.
Edit2: Also here a short excerpt from InputStreamResource documentation:
this is a descriptor for an already opened resource - therefore returning {@code true} from {@link #isOpen()}. Do not use an {@code InputStreamResource} if you need to keep the resource descriptor somewhere, or if you need to read from a stream multiple times.
I find it very misleading that your framework is designed to accept an already opened resource but not closing it. Please fix it or at the very least make your design decision very clear in the documentation.
Comment From: rstoyanchev
Wait, from a user point of view I don't even notice that.
That's an odd argument considering that you are opening an InputStream inside your controller method. When something like this is not in a try-catch-finally block there is no way to guarantee proper release. So if an error is encountered at any time between creation of the InputStream and 304 response handling, there would be a leak. This includes potential errors even in your own controller method, e.g. extra lines or logic between the creation of the InputStream and the return of ResponseEntity. It is simply not a good idea to open a stream like that in a controller method.
Also just not opening the resource in the controller because the framework decides somewhere later not to read it is not trivial at all.
Why couldn't it be as simple as this:
public class MyResource extends AbstractResource {
private final Supplier<InputStream> supplier;
// constructor
@Override
public InputStream getInputStream() throws IOException, IllegalStateException {
return supplier.get();
}
}
To me the excerpt from InputStreamResource is giving you a warning that you should be careful when using that. If anything I can see potential for adding a Supplier<InputStream> based Resource.
Comment From: mixcloud-downloader
Wait, from a user point of view I don't even notice that.
That's an odd argument considering that you are opening an InputStream inside your controller method.
Sorry I fail to follow you. Maybe here's a missunderstanding. I mean as a framework user, I was not aware of the framework behavior with streams and some header constellation. Is that clearly documentated or intuitive or why do you think I, as a user should be aware of the framework's behavior?
So if an error is encountered at any time between creation of the InputStream and 304 response handling, there would be a leak. This includes potential errors even in your own controller method, e.g. extra lines or logic between the creation of the InputStream and the return of ResponseEntity
Look, what happens in my Controller is my responsibility. I have control there and do take care about proper error cases. But, everything afterwards is framework land. My power ends there and all I can do is beg you to please close that stream. Please, just do that and your documentation of InputStreamResource would tell the truth.
I don't even understand what you are arguing about? Do you think the framework's behavior is correct here? Sometimes it closes the streams, and sometimes in obscure and undocumented cases we don't and the user is out of luck. Instead of fixing it you simply suggest not using this feature at all. I don't understand your motivation behind this.
Why couldn't it be as simple as this:
Not my use case. Sorry, I need to know if the stream works beforehand and get some informations out of it to send appropriate headers. Funny thing, those headers (e.g. ETag) do trigger the odd resource leaking framework behavior, which you are defending. I give up, don't fix it, if you think this is intuitive and intended design. Oh boy.
Comment From: rstoyanchev
I don't see how else to say it so I'll repeat. It's how resources work in Java. They must be opened and closed in a try-finally block. If you let a resource outside a try-finally block it's a leak, or at least a potential for a leak, and that's an application leak.
A documentation update would be something like this:
If you open a resource inside a controller method, please do it within a try-finally or try-with-resources and make sure it's closed before the controller method returns. It's the only way to guarantee the resource is closed. In other words we do not expect controllers to open resources and keep them open after returning unless the application had some mechanism in place to ensure the resource is closed.
Now back to your use case. If you must read the content of the file to choose headers, I'm afraid you'd have to handle that inside the controller method, e.g. taking the request and response as arguments, and using ResourceHttpMessageConverter to write to the response. The only other alternative is to open the resource, check what you need, select the headers, close the resource, and then return a deferred resource as I suggested above.
Funny thing, those headers (e.g. ETag) do trigger the odd resource leaking framework behavior, which you are defending.
ETag handling isn't triggering a leak. We just don't need to read the resource in that case and generally we don't expect that an application would let open resources go outside a try-finally block without being closed.
Comment From: mixcloud-downloader
I don't see how else to say it so I'll repeat. It's how resources work in Java. They must be opened and closed in a try-finally block.
You're wrong about that. If something is returning a resource, it simply can't close that. Check the Java API, you see on every corner code which is returning streams and it's outside of try-with-resource. Now what should happen is whoever is the receiver of such a stream, should close it. And this should is a strong should, as finalizers (those guys who close is nowadays for sloppy developers) are deprecated.
ETag handling isn't triggering a leak.
It (or last-modified) is from my perspective. How else would the framework be able to make that decision on the conditional http request?
Could you just for one minute take of your framework-developer hat and put on a user hat. A user of some random library. That library has somewhere a method which accepts an InputStream and it explicitly tells you that you give up control of that resource and you won't have any chance to close that stream after you passed it to that library. How would feel when you learn, that this library does most of the times close the stream, but only sometimes not?
But, I agree, there's no new input coming into this discussion. Unfortunately it's only us two and no @jhoeller would ever come to give maybe a third opinion on this supposed bug. So, I give up. Keep it.
Comment From: rstoyanchev
A user of some random library. That library has somewhere a method which accepts an InputStream and it explicitly tells you that you give up control of that resource and you won't have any chance to close that stream after you passed it to that library.
That's not at all a match for the situation. You're not passing an InputStream into a method. It's the exact opposite, i.e. returning an InputStream from a method. The framework would have to have a try-finally around that method invocation, which happens right here but as you can see the code there is pretty generic and is not aware of the nature of the return value (response body, response entity, model, view, etc) and it's not a good place to start checking things like is this a ResponseEntity and if so does it contain a Resource and if so is it an InputStreamResource, and if so make sure it's closed, but only after invoking HttpEntityMethodProcessor which handles that return value. It's just not a good place to do something like that.
But, I agree, there's no new input coming into this discussion.
In your previous comment you explained more about your use case and constraints. That was constructive, and I responded with further suggestions. You chose not to extend the conversation further.
Comment From: rstoyanchev
Also if we were to accept responsibility for dealing with open resources in the return value from a controller method in one case, we would have to assume all such cases which would include return asynchronous return values like DeferredResult<Resource> or DeferredResult<HttpEntity<Resource>> and the like, which would more or less be not possible since it involves an async dispatch through the servlet container that cannot be wrapped with try-finally.
Comment From: Paun-Mihai
Hello @rstoyanchev. I also wanted to somehow defer the InputStream creation and I've followed your example:
public class DeferredInputStreamResource extends AbstractResource {
private Supplier<InputStream> inputStreamSupplier;
public DeferredInputStreamResource(Supplier<InputStream> inputStreamSupplier) {
this.inputStreamSupplier = inputStreamSupplier;
}
@Override
public String getDescription(){
return "";
}
@Override
public InputStream getInputStream() {
return this.inputStreamSupplier.get();
}
@Override
public boolean isOpen() {
return true;
}
@Override
public boolean exists() {
return true;
}
}
Next the controller would call something like this:
return ResponseEntity.ok(new DefferedInputStreamResource(()->someClass.createInputStream()));
However, this is not working for me. It simply blocks and then throws this:
org.springframework.web.util.NestedServletException: Request processing failed; nested exception is java.lang.IllegalArgumentException: No InputStream specified
at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1014)
at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:898)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:634)
at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:883)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:741)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at brave.servlet.TracingFilter.doFilter(TracingFilter.java:65)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:320)
at org.springframework.security.web.access.intercept.FilterSecurityInterceptor.invoke(FilterSecurityInterceptor.java:126)
at org.springframework.security.web.access.intercept.FilterSecurityInterceptor.doFilter(FilterSecurityInterceptor.java:90)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:118)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:137)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:111)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:158)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.savedrequest.RequestCacheAwareFilter.doFilter(RequestCacheAwareFilter.java:63)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.oauth2.provider.authentication.OAuth2AuthenticationProcessingFilter.doFilter(OAuth2AuthenticationProcessingFilter.java:176)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:116)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.header.HeaderWriterFilter.doHeadersAfter(HeaderWriterFilter.java:92)
at org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:77)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:105)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:56)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:215)
at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:178)
at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:358)
at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:271)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:93)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.cloud.sleuth.instrument.web.ExceptionLoggingFilter.doFilter(ExceptionLoggingFilter.java:50)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at brave.servlet.TracingFilter.doFilter(TracingFilter.java:82)
at org.springframework.cloud.sleuth.instrument.web.LazyTracingFilter.doFilter(TraceWebServletAutoConfiguration.java:138)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.doFilterInternal(WebMvcMetricsFilter.java:108)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:202)
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96)
at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:541)
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139)
at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92)
at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
at org.apache.catalina.valves.RemoteIpValve.invoke(RemoteIpValve.java:747)
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343)
at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:367)
at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:860)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1598)
at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
at java.base/java.lang.Thread.run(Unknown Source)
Caused by: java.lang.IllegalArgumentException: No InputStream specified
at org.springframework.util.Assert.notNull(Assert.java:198)
at org.springframework.util.StreamUtils.copy(StreamUtils.java:136)
at org.springframework.http.converter.ResourceHttpMessageConverter.writeContent(ResourceHttpMessageConverter.java:137)
at org.springframework.http.converter.ResourceHttpMessageConverter.writeInternal(ResourceHttpMessageConverter.java:129)
at org.springframework.http.converter.ResourceHttpMessageConverter.writeInternal(ResourceHttpMessageConverter.java:45)
at org.springframework.http.converter.AbstractHttpMessageConverter.write(AbstractHttpMessageConverter.java:227)
at org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodProcessor.writeWithMessageConverters(AbstractMessageConverterMethodProcessor.java:298)
at org.springframework.web.servlet.mvc.method.annotation.HttpEntityMethodProcessor.handleReturnValue(HttpEntityMethodProcessor.java:226)
at org.springframework.web.method.support.HandlerMethodReturnValueHandlerComposite.handleReturnValue(HandlerMethodReturnValueHandlerComposite.java:82)
at org.springframework.hateoas.server.mvc.RepresentationModelProcessorHandlerMethodReturnValueHandler.handleReturnValue(RepresentationModelProcessorHandlerMethodReturnValueHandler.java:101)
at org.springframework.web.method.support.HandlerMethodReturnValueHandlerComposite.handleReturnValue(HandlerMethodReturnValueHandlerComposite.java:82)
at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:124)
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:888)
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:793)
at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87)
at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1040)
at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:943)
at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1006)
It works with the classic way:
return ResponseEntity.ok(new InputStreamResource(someClass.createInputStream());
What can be wrong in this case?
Comment From: rstoyanchev
It seems like your Supplier returns null?
Comment From: Paun-Mihai
This is strange because that method call someClass.createInputStream() never returns null.
I can say this because using return ResponseEntity.ok(new InputStreamResource(someClass.createInputStream()); works just fine.
The supplier is just a lambda calling the same method: return ResponseEntity.ok(new DefferedInputStreamResource(()->someClass.createInputStream()));
Comment From: wolfch-elsevier
So is this leak fixed? Does the framework close the stream after the client reads the content? Ok, it's fixed, thanks.
class LoggingFileInputStream extends FileInputStream {
final static Logger log = LoggerFactory.getLogger(LoggingFileInputStream.class);
public LoggingFileInputStream(File file)
throws FileNotFoundException {
super(file);
}
@Override
public void close() throws IOException {
log.info("================ FileInputStream closed ================");
super.close();
}
}