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

problems with Promise error handling in plugin registration #3381

Closed
5c077yP opened this issue Nov 21, 2016 · 3 comments
Closed

problems with Promise error handling in plugin registration #3381

5c077yP opened this issue Nov 21, 2016 · 3 comments
Assignees
Labels
Milestone

Comments

@5c077yP
Copy link

@5c077yP 5c077yP commented Nov 21, 2016

Hey,

I recently experienced an issue, when registering plugins using promise style and the plugin itself is not calling next with a proper Error Object. (Btw. I'm using the latest hapi version 15.2.0.)

Let's say I'm registering plugins like this

server.on('start', () => console.log('server started successfully'));

return server.register(plugins) // plugins is a list of plugins
  .then(() => server)
  .catch((err) => {
    console.errror('could not register plugins', err);
    process.exit(1);
  })

now imagine a bad plugin like this:

class BadError {
  constructor(message) {
    this.message = message;
  }
}

const badPlugin = {
  register(server, options, next) {
    return next(new BadError('bad-error'));
  }
};

badPlugin.register.attributes = { name: 'bad-plugin' };

and my list of plugins:

const plugins = [
  /* .. `set-of-plugins-1` */,
  { register: badPlugin },
  /* .. `set-of-plugins-2` */
]

What I thought is, that the server exits with code 1, but what happens is the server starts, logs server started successfully. Now the server keeps running and all set-of-plugins-1 are running but none of set-of-plugins-2.

I checked the code and what I can tell is, that when calling the server.register function without a callback it's using the Promise.wrap, which re-calls the function with a callback. The register function now iterates through all plugins using Items.serial, which itself breaks the serial flow whenever any argument is passed to the next callback. But now this "wrapped" promise callback only expects one argument, which if an Error is rejected otherwise resolved.

In the described case, the plugin is unfortunately not calling next with a proper Error Object. Now the Items.serials breaks the flow, it calls the wrapped promise callback, but this callback itself doesn't recognise the error and instead just resolves the Promise. In my case this leads to a situation where all plugins before the "bad" one are registered fine, but the rest not and the server keeps running.

In this case I'm not controlling the errors, but I could check the error and wrap it into a real Error Object. Another solution could be to check in the server.register().then() if any argument is passed to the then function it can only be an error (i think).

Of course it would be great if from hapi itself this would be reported as an error, maybe there should be a 4th argument to Promise.wrap (e.g. nodeStyleCallback = false) which if a first argument is given rejects and otherwise resolves with the second argument, because in the case of the plugin registration always only an error (or nothing) is passed to the callback, but of course i'm not super deeply into the code, so this is just an idea.

Thanks for reading.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 29, 2016

Is the issue here that a plugin returns a non Error error and that's causing the promise to fail?

@5c077yP
Copy link
Author

@5c077yP 5c077yP commented Nov 29, 2016

somehow. the plugin returns a non Error error. The plugin registration loop stops/fails, but the promise of the server.register is resolved instead of rejected.

@hueniverse hueniverse added the bug label Nov 29, 2016
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@5c077yP
Copy link
Author

@5c077yP 5c077yP commented Nov 29, 2016

thanks 👍

@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants