Skip to content

Fix resourse cleaning #570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from
Open

Conversation

Delagen
Copy link

@Delagen Delagen commented Mar 13, 2018

request stream by demand, fix fs.unlink to call when response send to client or stream open

@Delagen
Copy link
Author

Delagen commented Mar 13, 2018

For #568

@LinusU
Copy link
Member

LinusU commented Mar 14, 2018

Would you mind explaining more what this change does and why you think it's the right approach?

I fail to see how this could fix an EPERM error 🤔

@Delagen
Copy link
Author

Delagen commented Mar 14, 2018

@LinusU It does not fix EPERM error, it's my codebase error.
But it solve unneeded stream opening when code doesn't need it.
And remove files after handler ends and response finished even if files doesn't read in it.

Without this PR files are left opened till process exit if user handler do nothing with them

@LinusU
Copy link
Member

LinusU commented Mar 19, 2018

Hmm, I'm afraid that relying on this could have unintended consequences. For example, I'm not sure that the following code would still work:

app.post('/upload', upload.single('file'), async (req, res) => {
  res.send(200, 'OK')

  const fileName = await allocateFileName()
  const target = fs.createWriteStream()

  req.file.stream.pipe(target)
})

Basically, I'm not sure (I don't have a definitive decision, just want to bring up discussion) that not using the stream before responding to the http request means that you don't want to use the file. 🤔

@Delagen
Copy link
Author

Delagen commented Mar 19, 2018

@LinusU this code should work, but it wrong because it can crash, but user does not get any info about it. I recommend at least get stream before finishing request.
I only moved create stream to singleton with getter, and fix removing files at response finish.
But without my PR

app.post('/upload', upload.single('file'), async (req, res) => {
  res.send(200, 'OK')
})

will create tmp file and DOES NOT remove it at any time and takes handle, so app may reach open_files limit and crash.

@LinusU
Copy link
Member

LinusU commented Mar 28, 2018

this code should work

I don't think that it does, since the file will have been removed by the time it hits the req.file.stream getter.

but it wrong because it can crash, but user does not get any info about it

That doesn't necessarily make it wrong. There could be plenty of reasons for not wanting to notify the user of errors.

will create tmp file and DOES NOT remove it at any time and takes handle, so app may reach open_files limit and crash.

The files should be removed, but I just saw that this does in fact leak file descriptors. Basically it boils down to the underlying FD not beeing freed when the ReadStream is garbage collected.

This is potentially something that should be fixed in Node.js, I'll open an issue about that.

We could probably use e.g. node-weak to work around that manually for now.

@noonii
Copy link

noonii commented Mar 24, 2020

+1

I noticed in multer@next that OP is right it does not get deleted from temp file upon failed or successful request.

When I re-run the app, only then it is removed.

@UlisesGascon UlisesGascon added this to the 3.0.0 milestone May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants