search

ipfs / js-ipfs-repo

Implementation of the IPFS Repo spec in JavaScript
https://github.com/ipfs/specs/tree/master/repo
Other
79 stars 50 forks source link

proper-lockfile errors after sleep #188

Closed alanshaw closed 5 years ago

alanshaw commented 5 years ago
  1. Start an IPFS daemon
  2. Put your computer to sleep for 30s

  3. 💥

    /Users/alan/Code/protocol-labs/js-ipfs/node_modules/proper-lockfile/lib/lockfile.js:181
        onCompromised: (err) => { throw err; },
                                  ^

Error: Unable to update lock within the stale threshold
    at options.fs.utimes (/Users/alan/Code/protocol-labs/js-ipfs/node_modules/proper-lockfile/lib/lockfile.js:109:21)
    at FSReqWrap.oncomplete (fs.js:141:20)
achingbrain commented 5 years ago

Silly question, but since everything is immutable, why do we need to lock the repo? There would need to be some inter-process guarding around things like the MFS root hash or the pin sets to prevent simultaneous writes, but for the most part shouldn't it all be append-only?

achingbrain commented 5 years ago

Or are we just using the wrong data store?

hugomrdias commented 5 years ago

brainstorming this with @satazor (proper-lockfile creator)

jacobheun commented 5 years ago

Silly question, but since everything is immutable, why do we need to lock the repo? There would need to be some inter-process guarding around things like the MFS root hash or the pin sets to prevent simultaneous writes, but for the most part shouldn't it all be append-only?

We had previously looked at making locking optional, but there were concerns about doing that. I think a primary concern would be protecting against multi-tenancy issues, especially if we end up with different versions of go/js. I haven't done much exploration into this though.

brainstorming this with @satazor (proper-lockfile creator)

@hugomrdias any update on this? Do we need the staleness check? We previously didn't have it and it was only added with proper-lock. Perhaps an intermediary solution would be to do some checking in the onCompromised option and refresh the lock if it was a staleness issue. This isn't ideal, but it would at least prevent these staleness issues until we have an appropriate resolution.

satazor commented 5 years ago

We had a discussion last week. The issue is that proper-lockfile fails if the time it took to update the lock was greater than the threshold. Now, this doesn't necessarly mean that the lock was compromised but, at the moment, proper-lockfile throws. We have discussed in doing an extra check to prevent it from throwing. That will solve the sleep scenario as well as the one in which the event loop is blocked for quite some time.

alanshaw commented 5 years ago

@satazor do you think you'll be able to implement a fix soon? This issue is a big problem for anyone using a JS IPFS daemon with IPFS companion (amongst other reasons).

hugomrdias commented 5 years ago

@satazor will not be able to take this, i will implement the extra check in proper-lockfile

nick commented 5 years ago

I'm seeing the sleep issue too. Not sure how to help, though... if someone with more context could provide some pointers I'd be happy to take a look.

hugomrdias commented 5 years ago

can you please validate that this PR https://github.com/moxystudio/node-proper-lockfile/pull/74 fixes the problem pls ?

alanshaw commented 5 years ago

@hugomrdias would you mind submitting a PR here so I can do the same for js-ipfs?

hugomrdias commented 5 years ago

i will once i release proper-lockfile can link it in js-ipfs and validate the fix? i pushed some more commits to the branch just now

jacobheun commented 5 years ago

Should be resolve in v0.26.3