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

Strange behaviour with throw in handlers #2616

Closed
qraynaud opened this issue Jun 24, 2015 · 8 comments
Closed

Strange behaviour with throw in handlers #2616

qraynaud opened this issue Jun 24, 2015 · 8 comments
Assignees
Labels
Milestone

Comments

@qraynaud
Copy link

@qraynaud qraynaud commented Jun 24, 2015

Hi,

I'm working on latest, so v8.6.1.

I never noticed this before so maybe it's a recent bug, but when I throw in a handler, sometimes the error is sent to the wrong request.

Here is the route that fails:

'use strict';

module.exports = {
  method: 'GET',
  path: '/api/user/_me',
  config: {
    handler: function(request, reply) {
      var user = request.session.get('user');
      if (user)
        return reply(user);
      var err = new Error('You are disconnected');
      err.status = 401;
      throw err;
    }
  }
};

This is only a test to showcase the problem, on a normal day, I would just reply the error.

I don't know if it can be related, but I have the following onPreResponse handler:

// Handle errors
server.ext('onPreResponse', function(request, reply) {
  if (!(request.response instanceof Error) && !request.response.isBoom)
    return reply.continue();

  console.error(chalk.grey('[') + chalk.red('err') + chalk.grey('] on ') +
    chalk.yellow(request.path) + (request.url.search ? chalk.blue(request.url.search) : '') + chalk.grey(':'));
  console.error((request.response.stack || request.response).toString().replace(/^/mg, chalk.grey('> ')));

  reply(_.pick(request.response, ['message', 'status']))
    .code(request.response.status || 500)
  ;
});

I make multiple simultaneous requests using my browser. Most of the time it works but sometimes another request gets the error and I never get to have an answer on the throwing route.

Here is my network stack when it works:
api_success

And here is the result once every 5 times or so (this is random):
api_fail
As you can see, a random call to my i18n endpoint failed and I got no answer for my call to _me.

The failing API call response looks like:
response_on_fail
And here we can see that the error is the one emitted by _me that got no answer…

Any ideas of what I might be doing wrong?

Another possibility might be my i18n endpoint that is using the official elasticsearch client that might or might not restore the domains properly. Could the other endpoint mess with my _me route?

@qraynaud
Copy link
Author

@qraynaud qraynaud commented Jun 24, 2015

Seems that the elasticsearch client is only using http.Client and http.Agent to do its job so I think domains are properly handled by it… Not the cause it seems.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jun 24, 2015

Domains are shit.

You need to look into everything you are using in any extension, handler, pre, method, etc. that might have it's own client or domain context. Do you know where the error you do get comes from? It sounds like a handler is going through some async action that cross the domain boundary.

The problem is that we can't fix anything involving domains... so we need to find workarounds for you to prevent the issue from happening in the first place.

Loading

@mik01aj
Copy link

@mik01aj mik01aj commented Jul 27, 2015

@hueniverse if "domains are shit", as you say, what would you recommend for handling request errors in a generic way? I'm struggling with similar problems.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 7, 2015

@qraynaud any chance you are using server methods or any caching provided by hapi? We had a bug in catbox where the throw from the first request would be passed to all other pending requests and mess up the domain stack. Also, is there any overlap between the two route (common code, ext.) that uses elastic search? We need to find a place where the two domains of each request cross over with something else.

@mik01aj You just need to be very careful and have a good mapping of where your handler changes domains. It's hard. Most people don't hit the edge cases and when you do, like this, you need to dig deeper to see where you have overlapping context (such as two handlers both using the same HTTP connection and therefore crossing domains boundaries).

Loading

@hueniverse hueniverse added the bug label Aug 7, 2015
@hueniverse hueniverse self-assigned this Aug 7, 2015
@qraynaud
Copy link
Author

@qraynaud qraynaud commented Aug 9, 2015

@hueniverse : Yes, I was using catbox indeed. And no there is no overlap (other than catbox/yar) I can see between the 2 routes.

I suppose I will update catbox asap then. Thanks for the tip.

I'll keep in touch if this fixes the problem.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 9, 2015

This is not published yet. If you can manually try out the fix here: https://github.com/hapijs/catbox/pull/128/files before it is released, please report back if it solved your problem.

Loading

@hueniverse hueniverse closed this Aug 9, 2015
@hueniverse hueniverse reopened this Aug 9, 2015
@hueniverse hueniverse added this to the 9.0.0 milestone Aug 10, 2015
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 10, 2015

I'm going to assume this is related to the catbox issue. Once 9.0 is out please check again and if it is still not resolved, open a new issue. Thanks!

Loading

@hueniverse hueniverse closed this Aug 10, 2015
@qraynaud
Copy link
Author

@qraynaud qraynaud commented Aug 13, 2015

Ok thx. I'm sorry I have so little time for this right now.

Loading

@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 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
3 participants