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

Use fs-write-stream-atomic #127

Closed
sindresorhus opened this issue Oct 12, 2014 · 12 comments
Closed

Use fs-write-stream-atomic #127

sindresorhus opened this issue Oct 12, 2014 · 12 comments

Comments

@sindresorhus
Copy link
Owner

Currently we leave behind corrupt files if the process somewhat fails, this would prevent it.

https:/npm/fs-write-stream-atomic

@kevva
Copy link
Contributor

kevva commented Oct 14, 2014

+1

@kevva kevva closed this as completed in c3b4989 Oct 20, 2014
@sindresorhus
Copy link
Owner Author

Still not optimal. It now prevents race conflicts, but if the process exits early (eg. by the user force exiting) the temp file is left in place: todomvc.com-1000x1000.png.236b1396348943e83e939985535b221e.

It should probably be cleaned up.

@kevva Would it make sense to open a feature request on that module?

@kevva
Copy link
Contributor

kevva commented Oct 21, 2014

Does it matter if it's written to the temp dir though? But if it's fixable I don't see why they shouldn't do it. Might as well open a request.

@sindresorhus
Copy link
Owner Author

It's written to the cwd, not a temp dir. Try Ctrl+C a pageres session.

@sindresorhus sindresorhus reopened this Oct 21, 2014
@kevva
Copy link
Contributor

kevva commented Oct 21, 2014

Oh, they should at least write it to /tmp imo. I'll open an issue.

@sindresorhus
Copy link
Owner Author

Wanna open a ticket there?

@kevva
Copy link
Contributor

kevva commented Oct 21, 2014

Yup, npm/fs-write-stream-atomic#1.

@sindresorhus sindresorhus modified the milestone: 1.1.0 Nov 17, 2014
@sindresorhus
Copy link
Owner Author

I guess we could do it manually in our package by removing this.__atomicTmp on process.exit or process.SIGINT.

Let's do that for now. I don't see them fixing it in the near-term.

@kevva
Copy link
Contributor

kevva commented Nov 17, 2014

Hm, I tried to do this, but each-async freaks out because next is called multiple times since writeStreamAtomic will emit errors when the process is canceled.

@sindresorhus
Copy link
Owner Author

@kevva
Copy link
Contributor

kevva commented Nov 17, 2014

Yeah, but how do we circumvent it nicely? Calling cb instead of next works or by stop listening for errors on the writeStream (which is not ideal). I tried checking the error code for ENOENT and only calling cb when true but that didn't work for some weird reason.

@sindresorhus
Copy link
Owner Author

We use onetime to only care about the first error received.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants