Skip to content
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

Remove input file after exec (in all cases) #4

Closed
wants to merge 3 commits into from
Closed

Remove input file after exec (in all cases) #4

wants to merge 3 commits into from

Conversation

leachiM2k
Copy link

Hello,
I've ran into disk space issues after I've started to use the newest version of imagemin-jpegrecompress.

The problem is obviously that exec-buffer creates a file from input buffer, but does not delete it at any time.

This is my proposal for a fix.

I wanted to write a test for that, but this is not as easy, because there is no hint of the temp file from the outside. If you do know how to achieve this, let me now.

@leachiM2k
Copy link
Author

@kevva Do you think you can merge this?

@SamVerschueren
Copy link
Collaborator

I'd suggest creating an unlink function like this

const unlink = (filePath, err) => fsP.unlink(filePath).then(() => {
    if (err) {
        throw err;
    }
});

Would be a lot cleaner to integrate this

    return fsP.writeFile(inputPath, opts.input)
        .then(() => execa(opts.bin, opts.args))
        .catch(err => unlink(inputPath, err))
        .then(() => unlink(inputPath))
        .then(() => fsP.readFile(outputPath));

@leachiM2k
Copy link
Author

My Pull Request is only a suggestion. Obviously, I could have opened an issue only. But I tried to fix that.
I don't know why this problem is not that severe. Every imagemin-* package uses exec-buffer in the end and should have this disk space problem.

@SamVerschueren
Copy link
Collaborator

Did I miss something here? I don't say your PR is no good. I believe you are right that the file should be removed even when an error occurs. The code I gave does exactly the same thing but is in my opinion just cleaner and wanted to share that.

But it's up to @kevva to decide what he wants.

@kevva
Copy link
Owner

kevva commented Aug 24, 2016

I agree with @SamVerschueren, just adapt your PR according to his suggestion and I'll merge.

@leachiM2k
Copy link
Author

Hey @kevva and @SamVerschueren, sorry for my overreaction.
While adapting the suggestion, I've realized, that exec-buffer should remove both input and output files in any case.
So, I've combined that functionality and @SamVerschueren 's suggestion.

I hope it is good enough now.

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.

None yet

3 participants