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

Fix uncaught init errors #38

Closed
wants to merge 4 commits into from

Conversation

millette
Copy link
Contributor

Without this patch, db initialization errors can go uncaught. I added a test to demonstrate, using "/test" location where it should be impossible to write.

I'm really not used to tap and I expect there's a much better way to write the test with t.throws or t.rejects. Any help on that end would be appreciated.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@millette
Copy link
Contributor Author

Note that since the plugin requires leveldown >= 5 and it require node >= 8.6.0 I also patched this plugins' requirement. And since we have node > 8, I'm using util.promisify.

See Level/leveldown@d08c48f

next()
next()
})
.catch(next)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping Node 6 is not needed here.

levelMore(opts.name, opts.options, function (err, db) {
  if (err) {
    next(err)
    return 
  }
  fastify.decorate('level', db)
  fastify.addHook('onClose', close)
  next()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But leveldown itself requires >= 8.6.0.

I can revert the change, but I'll also have to remove util.promisify().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok. I would prefer to avoid promisify anyway as there is no need to use promises here.

.register(level, { name: '/test' })
.ready(err => {
t.ok(err)
t.equal(err.message, 'IO error: /test/LOCK: No such file or directory')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test would pass on Windows.
It is also trying to write to a file at the root of the file system, which is a bad practice.
I think you can get a similar error if you create a temp file (not directory) and try to open it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to write somewhere impossible (no permission), but I could trigger an error by setting createIfMissing = false in the leveldown options and use "test" like in the other places.

Would that be ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be ok as well, yes.

@delvedor
Copy link
Member

Closed in #40.

@delvedor delvedor closed this Apr 25, 2020
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