Skip to content

fix: fix repo lock and buffer api #185

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

Merged
merged 2 commits into from
Dec 13, 2018
Merged

fix: fix repo lock and buffer api #185

merged 2 commits into from
Dec 13, 2018

Conversation

hugomrdias
Copy link
Member

This PR makes the repo lock safer with callback/promise conversion, using then second arg we don't catch error inside the first arg fn and avoid callback already called errors.

Plus changes 2 two test to use the new buffer api.

@hugomrdias hugomrdias requested a review from jacobheun December 10, 2018 14:32
@ghost ghost assigned hugomrdias Dec 10, 2018
@ghost ghost added the status/in-progress In progress label Dec 10, 2018
@achingbrain
Copy link
Member

achingbrain commented Dec 10, 2018

What happens when the invocation of callback inside the .then throws? I think you'll end up with an unhandled promise rejection:

// index.js
const callback = () => {
  throw new Error('Derp')
}

const work = () => {
  return Promise.resolve()
}

work()
  .then(() => {
    callback()
  }, () => {
  console.info('never invoked')
})
$ node index.js
(node:72480) UnhandledPromiseRejectionWarning: Error: Derp
    at callback (/path/to/index.js:2:9)
    at work.then (/path/to/index.js:11:5)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
(node:72480) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:72480) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@hugomrdias
Copy link
Member Author

yes true but at least get to see the error, right now the stack start at callOnlyOnce inside async and the actual error isn't there.
This will get better with the async iterators work and for now either we handle the error top level or if it throws like in your example its probably a bug on our side and we can find the real problem easier.

@hugomrdias
Copy link
Member Author

should be good now @jacobheun

@jacobheun
Copy link
Contributor

@hugomrdias your original intent was to avoid the callback twice error, correct? This is just logging whatever error we get now, but still has a risk of that. Shouldn't this look like the following? This will avoid the double callback and catch and log any error if it's thrown from the callback.

  lock(dir, { lockfilePath: file, stale: STALE_TIME })
    .then(release => {
      callback(null, {
        close: (cb) => {
          release()
            .then(() => cb())
            .catch(err => cb(err))
        }
      })
    }, callback)
    .catch(err => {
      log(err)
    })

@hugomrdias
Copy link
Member Author

done

@jacobheun jacobheun merged commit f56aea3 into master Dec 13, 2018
@ghost ghost removed the status/in-progress In progress label Dec 13, 2018
@jacobheun jacobheun deleted the fix/repo-lock branch December 13, 2018 15:31
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.

3 participants