Repository: nuxt/nuxt.js
Pull request #5347 (2019-03-22 19:21:14)

fix(cli): catch all possible errors thrown from proper-filelock
<!--- Provide a general summary of your changes in the title above -->

![image](https://user-images.githubusercontent.com/1067403/54847600-111c5c00-4cdf-11e9-87c3-d473bda3c054.png)

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (a non-breaking change which fixes an issue)
- [ ] New feature (a non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)


## Description
<!--- Describe your changes in detail -->
<!--- Why is this change required? What problem does it solve? -->
<!--- If it resolves an open issue, please link to the issue here. For example "Resolves: #1337" -->
Ref: #5324

Although I could have expected it maybe, its not really documented that proper-lockfile also throws errors when it cant release a lock. This pr should fix that behaviour by wrapping any code which originates from proper-lockfile in a `try/catch` and warn when an error was thrown.

The _only_ time a lock should result in a `Nuxt Fatal Error` is when a lock with the same id already exists.

## Checklist:
<!--- Put an `x` in all the boxes that apply. -->
<!--- If your change requires a documentation PR, please link it appropriately -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly. (PR: #)
- [x] I have added tests to cover my changes (if not applicable, please state why)
- [ ] All new and existing tests are passing.



codecov-io: # [Codecov](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=h1) Report
> Merging [#5347](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=desc) into [dev](https://codecov.io/gh/nuxt/nuxt.js/commit/3c50876d737ced86f2e3677c1ee7be6328dcdcc9?src=pr&el=desc) will **decrease** coverage by `44.73%`.
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/graphs/tree.svg?width=650&token=nGD1PtMB3M&height=150&src=pr)](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##              dev    #5347       +/-   ##
===========================================
- Coverage   95.66%   50.92%   -44.74%     
===========================================
  Files          74       74               
  Lines        2513     2529       +16     
  Branches      639      641        +2     
===========================================
- Hits         2404     1288     -1116     
- Misses         92      962      +870     
- Partials       17      279      +262
```


| [Impacted Files](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/utils/src/locking.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvdXRpbHMvc3JjL2xvY2tpbmcuanM=) | `4.76% <0%> (-95.24%)` | :arrow_down: |
| [packages/generator/src/generator.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvZ2VuZXJhdG9yL3NyYy9nZW5lcmF0b3IuanM=) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/typescript/src/index.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvdHlwZXNjcmlwdC9zcmMvaW5kZXguanM=) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/index.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9pbmRleC5qcw==) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/options/server.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9vcHRpb25zL3NlcnZlci5qcw==) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/run.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9ydW4uanM=) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/commands/start.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9jb21tYW5kcy9zdGFydC5qcw==) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/commands/generate.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9jb21tYW5kcy9nZW5lcmF0ZS5qcw==) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/options/common.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy9vcHRpb25zL2NvbW1vbi5qcw==) | `0% <0%> (-100%)` | :arrow_down: |
| [packages/cli/src/utils/constants.js](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree#diff-cGFja2FnZXMvY2xpL3NyYy91dGlscy9jb25zdGFudHMuanM=) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [40 more](https://codecov.io/gh/nuxt/nuxt.js/pull/5347/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=footer). Last update [3c50876...c713b3f](https://codecov.io/gh/nuxt/nuxt.js/pull/5347?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packages/utils/src/locking.js b/packages/utils/src/locking.js
index d0ee5f0f0..c9b7fc15b 100644
--- a/packages/utils/src/locking.js
+++ b/packages/utils/src/locking.js
@@ -35,16 +35,33 @@ export async function getLockPath(config) {
 export async function lock({ id, dir, root, options }) {
   const lockPath = await getLockPath({ id, dir, root })
 
-  const locked = await properlock.check(lockPath)
-  if (locked) {
-    consola.fatal(`A lock with id '${id}' already exists on ${dir}`)
+  try {
+    const locked = await properlock.check(lockPath)
+    if (locked) {
+      consola.fatal(`A lock with id '${id}' already exists on ${dir}`)
+    }
+  } catch (e) {
+    consola.debug(`Check for an existing lock with id '${id}' on ${dir} failed`, e)
   }
 
-  options = getLockOptions(options)
-  const release = await properlock.lock(lockPath, options)
+  let lockWasCompromised = false
+  let release
+
+  try {
+    options = getLockOptions(options)
+
+    const onCompromised = options.onCompromised
+    options.onCompromised = (err) => {
+      onCompromised(err)
+      lockWasCompromised = true
+    }
+
+    release = await properlock.lock(lockPath, options)
+  } catch (e) {}
 
   if (!release) {
     consola.warn(`Unable to get a lock with id '${id}' on ${dir} (but will continue)`)
+    return false
   }
 
   if (!lockPaths.size) {
@@ -59,8 +76,27 @@ export async function lock({ id, dir, root, options }) {
   lockPaths.add(lockPath)
 
   return async function lockRelease() {
-    await release()
-    await fs.remove(lockPath)
-    lockPaths.delete(lockPath)
+    try {
+      await fs.remove(lockPath)
+      lockPaths.delete(lockPath)
+
+      // release as last so the lockPath is still removed
+      // when it fails on a compromised lock
+      await release()
+    } catch (e) {
+      if (!lockWasCompromised || !e.message.includes('already released')) {
+        consola.debug(e)
+        return
+      }
+
+      // proper-lockfile doesnt remove lockDir when lock is compromised
+      // removing it here could cause the 'other' process to throw an error
+      // as well, but in our case its much more likely the lock was
+      // compromised due to mtime update timeouts
+      const lockDir = `${lockPath}.lock`
+      if (await fs.exists(lockDir)) {
+        await fs.remove(lockDir)
+      }
+    }
   }
 }


Test patch:
diff --git a/packages/utils/test/locking.test.js b/packages/utils/test/locking.test.js
index 225c64157..52acfd6dd 100644
--- a/packages/utils/test/locking.test.js
+++ b/packages/utils/test/locking.test.js
@@ -61,7 +61,7 @@ describe('util: locking', () => {
   })
 
   test('lock creates a lock and returns a release fn', async () => {
-    properlock.lock.mockImplementationOnce(() => true)
+    properlock.lock.mockReturnValue(true)
 
     const fn = await lock(lockConfig)
 
@@ -75,7 +75,7 @@ describe('util: locking', () => {
   })
 
   test('lock throws error when lock already exists', async () => {
-    properlock.check.mockImplementationOnce(() => true)
+    properlock.check.mockReturnValue(true)
 
     await lock(lockConfig)
     expect(properlock.check).toHaveBeenCalledTimes(1)
@@ -84,7 +84,19 @@ describe('util: locking', () => {
   })
 
   test('lock logs warning when it couldnt get a lock', async () => {
-    properlock.lock.mockImplementationOnce(() => false)
+    properlock.lock.mockReturnValue(false)
+
+    const fn = await lock(lockConfig)
+    expect(fn).toBe(false)
+    expect(properlock.lock).toHaveBeenCalledTimes(1)
+    expect(consola.warn).toHaveBeenCalledTimes(1)
+    expect(consola.warn).toHaveBeenCalledWith(`Unable to get a lock with id '${lockConfig.id}' on ${lockConfig.dir} (but will continue)`)
+  })
+
+  test('lock logs warning when proper.lock threw error', async () => {
+    properlock.lock.mockImplementation(() => {
+      throw new Error('test error')
+    })
 
     await lock(lockConfig)
     expect(properlock.lock).toHaveBeenCalledTimes(1)
@@ -94,7 +106,7 @@ describe('util: locking', () => {
 
   test('lock returns a release method for unlocking both lockfile as lockPath', async () => {
     const release = jest.fn()
-    properlock.lock.mockImplementationOnce(() => release)
+    properlock.lock.mockImplementation(() => release)
 
     const fn = await lock(lockConfig)
     await fn()
@@ -105,7 +117,7 @@ describe('util: locking', () => {
 
   test('lock release also cleansup onExit set', async () => {
     const release = jest.fn()
-    properlock.lock.mockImplementationOnce(() => release)
+    properlock.lock.mockImplementation(() => release)
 
     const fn = await lock(lockConfig)
     expect(lockPaths.size).toBe(1)
@@ -114,8 +126,58 @@ describe('util: locking', () => {
     expect(lockPaths.size).toBe(0)
   })
 
+  test('lock release only logs error when error thrown', async () => {
+    const release = jest.fn(() => {
+      throw new Error('test error')
+    })
+    properlock.lock.mockImplementation(() => release)
+
+    const fn = await lock(lockConfig)
+    await expect(fn()).resolves.not.toThrow()
+
+    expect(consola.debug).toHaveBeenCalledTimes(1)
+  })
+
+  test('lock check only logs error when error thrown', async () => {
+    const testError = new Error('check error')
+    properlock.lock.mockImplementation(() => () => {})
+    properlock.check.mockImplementation(() => {
+      throw testError
+    })
+
+    const fn = await lock(lockConfig)
+    expect(fn).toEqual(expect.any(Function))
+
+    expect(consola.debug).toHaveBeenCalledTimes(1)
+    expect(consola.debug).toHaveBeenCalledWith(`Check for an existing lock with id '${lockConfig.id}' on ${lockConfig.dir} failed`, testError)
+  })
+
+  test('lock release doesnt log error when error thrown because lock compromised', async () => {
+    fs.exists.mockReturnValue(true)
+    const testError = new Error('Lock is already released')
+    const release = jest.fn(() => {
+      throw testError
+    })
+
+    properlock.lock.mockImplementation((path, options) => {
+      options.onCompromised()
+      return release
+    })
+
+    const fn = await lock({
+      ...lockConfig,
+      options: {
+        // overwrite default compromised which calls consola.warn
+        onCompromised() {}
+      }
+    })
+
+    await expect(fn()).resolves.not.toThrow()
+    expect(consola.warn).not.toHaveBeenCalled()
+  })
+
   test('lock sets exit listener once to remove lockPaths', async () => {
-    properlock.lock.mockImplementationOnce(() => true)
+    properlock.lock.mockReturnValue(true)
 
     await lock(lockConfig)
     await lock(lockConfig)
@@ -124,8 +186,10 @@ describe('util: locking', () => {
   })
 
   test('exit listener removes all lockPaths when called', async () => {
+    properlock.lock.mockReturnValue(true)
+
     let callback
-    onExit.mockImplementationOnce(cb => (callback = cb))
+    onExit.mockImplementation(cb => (callback = cb))
 
     const lockConfig2 = Object.assign({}, lockConfig, { id: 'id2' })
 
@@ -145,7 +209,7 @@ describe('util: locking', () => {
   })
 
   test('lock uses setLockOptions to set defaults', async () => {
-    const spy = properlock.lock.mockImplementationOnce(() => true)
+    const spy = properlock.lock.mockReturnValue(true)
 
     await lock(lockConfig)