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

usage of write-file-atomic #183

Open
mcollina opened this issue Dec 5, 2018 · 4 comments
Open

usage of write-file-atomic #183

mcollina opened this issue Dec 5, 2018 · 4 comments
Labels
status/ready Ready to be worked

Comments

@mcollina
Copy link

mcollina commented Dec 5, 2018

Writing file atomically is quite important for lock files and things things like that, however it should hardly be needed for all files operation. In the datastore-fs implementation, all files are written using the write-file-atomic module. As you can see in its source code, it does quite a few file system operations.

See https://github.com/ipfs/js-datastore-fs/blob/a6fc16ab4bd4c0b5d4189591057ae45e0d239908/src/index.js#L13 where write-file-atomic is used.

I think the best goal here is to add an option to store.put() to note that if file should be locked or not before usage. The flag could be ignored in other storage systems.

@daviddias daviddias added the status/ready Ready to be worked label Dec 9, 2018
@daviddias
Copy link
Member

Writing file atomically is quite important for lock files and things things like that, however it should hardly be needed for all files operation

Agreed. I believe this write-file-atomic got in for all operations because users were reporting doing multiple sequential ipfs.add and ipfs.get and although the first one succeeded, the second one failed because the files didn't have the chance to get flushed to disk.

An alternative to this that will also improve perf is to have a warm in memory cache of all the recent writes so that nextTick reads hit the cache rather then having to go all the way down to the disk.

@achingbrain
Copy link
Member

achingbrain commented Dec 10, 2018

users were reporting doing multiple sequential ipfs.add and ipfs.get and although the first one succeeded, the second one failed because the files didn't have the chance to get flushed to disk

That's interesting - it'd be good to see if this is still a problem with the latest codebase without write-file-atomic. Are there some old issues somewhere about this?

Did a quick search of ipfs/js-ipfs but didn't turn anything up.

@daviddias
Copy link
Member

From IRC

image

image

@daviddias
Copy link
Member

We need:

  • Have a repo config/policy that lets the user specify (with sane defaults)
    • To guarantee that every write is flushed
    • To have an in memory cache for faster confirmation times/reads
  • Agree on what are the sane defaults (might depend by runtime and OS, e.g. difference between two OS or OS vs Browser)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

3 participants