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

Exception from hook not handled #257

Open
thomasthiebaud opened this issue Dec 7, 2016 · 17 comments
Open

Exception from hook not handled #257

thomasthiebaud opened this issue Dec 7, 2016 · 17 comments
Labels

Comments

@thomasthiebaud
Copy link

I would like to validate my request (get, post, update or delete), and if the request is not valid, I would like to return an error (status 400 or 500 depending on the error). For the moment I'm using hooks for this task like this

const fortune = require('fortune');
const status = require('./model').status;
const { errors: { BadRequestError } } = fortune;

function input(context, record, update) {
  switch (context.request.method) {
    case 'create':
      if (status.indexOf(record.status) === -1) {
        throw new BadRequestError('Invalid status');
      } else {
        return record;
      }
    case 'update':
      if (!update || !update.replace || !update.replace.status) {
        return update;
      }

      if (status.indexOf(update.replace.status) === -1) {
        throw new BadRequestError('Invalid status');
      } else {
        return update;
      }
    default: return null;
  }
}

The important line here is throw new BadRequestError('Invalid status');. This line makes fortune returning a 400 error with invalid status as a content.

Even if I get the desire response, it seems that the error is not fully catch, and so I also get the error in the catch of the listener.

app.use((req, res) => createListener(req, res).catch(err => logger.error(err)));

So here are my questions:

  • Is it a desire behaviour ?
  • Is it the correct way to validate an entry in the hook ?
  • Is it possible to handle exceptions from hooks into fortunejs ?
@gr0uch
Copy link
Member

gr0uch commented Dec 7, 2016

It's intended behavior that the error appears again from the listener function. Internally the error gets caught and thrown again. This makes it possible to correlate the Node.js HTTP request with the error handled by Fortune.js.

That looks like how errors are meant to be handled. After throwing an error there's nothing more that can be done, what Fortune.js will do is immediately end the transaction if there is one and reject the request.

@thomasthiebaud
Copy link
Author

Thank you very much for your answer

@thomasthiebaud
Copy link
Author

One more thing, even if I catch the error on this line

app.use((req, res) => createListener(req, res).catch(err => /*Do something here*/));

I can still see

(node:20623) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status

in the logs. Also if I replace this line

app.use((req, res) => createListener(req, res).catch(err => logger.error(err)));

by this one

app.use(createListener);

I can see in the logs the error twice.

(node:20694) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status
(node:20694) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): BadRequestError: Invalid status

@thomasthiebaud thomasthiebaud reopened this Dec 7, 2016
@gr0uch
Copy link
Member

gr0uch commented Dec 8, 2016

That would probably be a bug somewhere in your application code. The tests for fortune-http work fine and don't emit warnings. Here's the server from the test:

const server = http.createServer((request, response) =>
  listener(request, response)
  .catch(error => stderr.error(error)))

Is it possible that the framework you're using for app accepts a return value from use and does something with it?

@thomasthiebaud
Copy link
Author

I'm using express. I setup it using the example here

const http = require('fortune-http');

const store = fortune({
  // Put models here
}, {
  hooks: {
    customer: [customerHooks.input],
    rule: [ruleHooks.input],
    user: [userHooks.input, userHooks.output],
  },
  adapter,
});

const api = http(store, {
  serializers: [
    [jsonApiSerializer],
  ],
});

const app = express();

/*
 * Use different middleware 
 * app.use(...);
 * app.use(...);
 */

app.use((req, res) => api(req, res).catch(err => logger.error(err)));


@gr0uch
Copy link
Member

gr0uch commented Dec 8, 2016

Hmm, maybe you could try two things:

  • Use http.createServer directly just to see if the warning still occurs.
  • Wrap the arrow function with brackets to avoid returning the promise to use, i.e. app.use((req, res) => { ... })

@thomasthiebaud
Copy link
Author

The problem remains the same. I tried :

const http = require('http');
const app = http.createServer((request, response) =>
  api(request, response)
  // Make sure to catch Promise rejections.
    .catch(error => {
      console.error(error.stack)
    }))

and I get

(node:30556) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): BadRequestError: Invalid status
BadRequestError: Invalid status
    at /home/thomasthiebaud/Combain/mate-v2-server/node_modules/fortune/lib/dispatch/update.js:100:13
    at process._tickCallback (internal/process/next_tick.js:103:7)

@gr0uch
Copy link
Member

gr0uch commented Dec 8, 2016

I can't reproduce the warning you're getting, throwing errors is working fine. Can you tell me which version you're using?

@thomasthiebaud
Copy link
Author

I think I'm using the latest version

"fortune": "5.0.2",
"fortune-http": "1.0.3",
"fortune-json-api": "2.1.0",
"fortune-postgres": "1.4.7",

@kketch
Copy link
Member

kketch commented Dec 9, 2016

@thomasthiebaud I tried to reproduce your issue with a similar code and did not get the uncaught exception, with node v7.2.0 :
https://gist.github.com/kketch/b7a84bb830bd071e672fbe1c52cfd071

And your stack trace seems weird:
https://github.com/fortunejs/fortune/blob/5.0.2/lib/dispatch/update.js#L100

As you can see this line do not throw a BadRequestError with a "Invalid status" message. Is it your complete stack trace ? this error seems to be thrown in the application code you shared earlier in the issue, we only see fortune

@thomasthiebaud
Copy link
Author

You are right, the stack trace is strange. I will try to investigate on this strange stack trace next week. Thank you very much for your feedbacks.

@gr0uch gr0uch added the bug label Feb 6, 2017
@cooperka
Copy link
Contributor

cooperka commented Dec 13, 2017

@thomasthiebaud did you ever find the cause of the warnings? I'm getting the same thing:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): BadRequestError: The value of "state" is invalid, it must be a "RequestState".

with very similar code as you (using http instead of express). The original error is being thrown by Fortune in this case during type validation; I don't expect I should need to handle that sort of thing.

The warnings are also reproducible with https://github.com/redoPop/fortunejs-example/.


Debug notes:

Trace when using --trace-warnings:

BadRequestError
    at checkValue (/home/demo/node_modules/fortune/lib/record_type/enforce.js:128:28)
    at enforce (/home/demo/node_modules/fortune/lib/record_type/enforce.js:77:12)
    at /home/demo/node_modules/fortune/lib/dispatch/create.js:86:9
    at map (/home/demo/node_modules/fortune/lib/common/array/map.js:14:14)
    at /home/demo/node_modules/fortune/lib/dispatch/create.js:81:26
    at <anonymous>

If I comment out the re-throw in lib/dispatch/index.js, the warning changes slightly:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'payload' of undefined
    at JsonApiSerializer.processResponse (/home/demo/node_modules/fortune-json-api/lib/index.js:100:36)
    at /home/demo/node_modules/fortune-http/lib/index.js:357:12
    at <anonymous>

@kketch
Copy link
Member

kketch commented Dec 13, 2017

Hi @cooperka, what should I do to reproduce exactly ? (with your example repository)

@gr0uch
Copy link
Member

gr0uch commented Dec 13, 2017

OK, I'll do a more thorough investigation then, the interesting part is TypeError: Cannot read property 'payload' of undefined.

@cooperka
Copy link
Contributor

@kketch with that example repo, simply start the server and hit it with any invalid request that causes some sort of error to be thrown and you'll see the warnings in your console.

@daliwali to add more info, it looks like the follow-up warning is thrown in fortune-json-api. I've added the full stack above. It may not be relevant since the error is supposed to have been re-thrown; more just useful to know that dispatch/index.js may be where the unhandled error is originating from.

@gr0uch
Copy link
Member

gr0uch commented Dec 16, 2017

I found the problem: the listener returns a promise that doesn't get a catch. The readme in fortune-http states:

const server = http.createServer((request, response) =>
  listener(request, response)
  // Make sure to catch Promise rejections.
  .catch(error => {
    console.error(error.stack)
  }))

I'm not sure what to do about it. I think it's somewhat unexpected for there to be a return value from the listener, I guess I could remove it but it would be a breaking change. Or I can make sure that this listener never throws.

Though I would be in favor of removing it and bumping major version, not sure what I was thinking with this, it makes dealing with HTTP-specific requests easier but it's kind of unexpected.

@cooperka
Copy link
Contributor

Hmm, in my opinion it makes sense as-is, no changes to the libraries needed -- I updated my server code to match your example above and it works great!

If you're good with it, I'm happy to make a PR to that example app to encourage others to follow the same pattern. I see now that this pattern is already in an example in the official guide, I just hadn't noticed the difference and blindly followed the boilerplate app. Thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants