StreamableFile pipe can leak resources if response is closed/errored prematurely

alexd6631 commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

https://github.com/nestjs/nest/blob/d83edc32dd61b84ca759487dd4aa017069549ea8/packages/platform-express/adapters/express-adapter.ts#L81

When returning a StreamableFile from a controller, it seems the express adapter will .pipe() it to the express response stream. As you may know pipe method is somewhat deprecated because it does not properly closes streams in case of abnormal termination, newer code should prefer use pipeline which automatically closes source stream on destination stream error.

So if a client cancels a request while it is streaming a StreamableFile, the stream wrapped by the StreamableFile will be kept forever opened, leading to a potential ressource leak.

Minimum reproduction code

WIP (see below)

Steps to reproduce

I am noticing the issue on a production app I am currently working on and I can't publish the code source without some adaptation effort to create the minimum reproduction code.

In my setup I have an endpoint that streams images store on S3 using the official AWS S3 client and StreamableFile on the controller side. The S3 client use keep alive and a pool of sockets, so if a GetObject stream is not fully consumed it will leak the socket and prevent it to return it to the pool.

I have a front that displays these images, and If I refresh the page several times quickly, it will cancel some in-flights requests on the server, but due to the .pipe behavior, some S3 Stream will not be destroyed. When the whole client pool is "poisoned", all S3 call will now hang forever, which is quite serious condition, only solved by restarting the nest server.

I could work on a minimum reproduction code, but the above setup is already quite involved. Let me know if there is enough information, or how "minimal" the reproduction code should be in my case.

Expected behavior

StreamableFile should destroy the underlying stream not only on full consumption but also in case of error / early abortion of the consumer stream. This should be achievable easily by replacing .pipe() method by the pipeline function

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[ ] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[X] code>@nestjs/platform-express</code
[X] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] Other (see below)

Other package

No response

NestJS version

8.4.5

Packages versions

Node.js version

16.15.0

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

No response

micalevisk commented 2 years ago

PRs are more than welcomed :smile_cat:

kamilmysliwiec commented 2 years ago

@jmcdo29 I think this is related to your PR (that introduces the StreamableFile). Do you think we can safely switch to using pipeline instead of .pipe?

jmcdo29 commented 2 years ago

I think so, and we have tests set up for the feature so we should be able to see if it breaks. I'll take a pass at it today and see what we're looking at.

jmcdo29 commented 2 years ago

Okay, so pipeline works well. I wanted to get your ideas on an enhancement here:

pipeline takes in the source, and modifiers (none needed here), the destination (response) and a callback. In that callback, if an error occurs it shows up here. Something we could probably do, is if an error happens then call res.status(400).send(err.message), instead of letting a 500 happen. We could also enhance the StreamableFile API to allow for setting of an error handler that would take in the res object and the error that occurs so that people could then implement there own error handling of the stream. This is better than the current 500 to server crash, which is a win. I unfortunately do not see a way for us to get the error to make its way over to the ExceptionFilter due to the error happening in the http adapter.

@kamilmysliwiec do you have any ideas or thoughts here?

kamilmysliwiec commented 2 years ago

Something we could probably do, is if an error happens then call res.status(400).send(err.message), instead of letting a 500 happen.

I think this would be reasonable yeah.

We could also enhance the StreamableFile API to allow for setting of an error handler that would take in the res object and the error that occurs so that people could then implement there own error handling of the stream

This could be a nice addition as well! Optional error handler that let devs provide their own custom implementation. I t think it's OK if we keep it separate from exception filters

kamilmysliwiec commented 2 years ago

Let's track this here https://github.com/nestjs/nest/pull/9819

nestjs / nest