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

Server fails to reuse Boom object #3378

Closed
wwalser opened this issue Nov 12, 2016 · 22 comments
Closed

Server fails to reuse Boom object #3378

wwalser opened this issue Nov 12, 2016 · 22 comments
Assignees
Labels
Milestone

Comments

@wwalser
Copy link
Contributor

@wwalser wwalser commented Nov 12, 2016

I am re-creating this issue which was opened hapijs/boom#140 which is more likely an issue inside Hapi. I've seen the same behavior on an older vertion of Hapi and Boom (see original issue for my comment).

------- pasted issue -------

I recently started a new project and since I have used Hapi before and was happy with it, I used it again.

I have everything modularized, nice and pretty. The project includes a handlers folder with files that each contains handlers for the same resource. For example, in the same file, I would have a handler to get a resource by its ID and another one that would update a resource by its ID. Since I want handlers of the same resource to return the same errors (in the previous example, an example would be a Boom.notFound('Resource was not found');), I store them in a const and use the const where needed.

In my previous project, this gave me no issue, but it does now. The issue is that the server becomes unresponsive after the second call. Trying to find the source of the problem, I came up with the following code that reproduces the issue:

'use strict';

const Boom = require('boom');
const Hapi = require('hapi');

const ERROR_TEST = Boom.forbidden('Error Test');

const server = new Hapi.Server();
server.connection({ port: 3000 });

server.route({
    method: 'GET',
    path: '/',
    handler: function (request, reply) {
        reply(ERROR_TEST);
    }
});

server.start((err) => {

    if (err) {
        throw err;
    }
    console.log(`Server running at: ${server.info.uri}`);
});

With this code, I call the server and I get an error response, as expected. If I call it again, there's no response from the server. If I call it a third time, there's no response from the server. All subsequent calls return no response.

If I replace the const for its value, everything works, so I have been changing my code accordingly, but I know that in the previous project the server worked without the replacement. This indicates me that there was a bug introduced in later versions of either hapi or more likely, boom.

The problem is not solely with Boom.forbidden(). I have tried the same code with Boom.badRequest(), Boom.unauthorized(), Boom.paymentRequired(), and Boom.notFound(). I'm guessing that the other errors will behave similarly.

Old Project

node version: v6.0.0
hapi version: ^13.5.0
boom version: ^3.2.1
os: Debian Jessie
Inside a Docker Container

New Project

node version: v6.0.0
hapi version: ^15.0.3
boom version: ^4.0.0
os: Debian Jessie
Inside a Docker Container
Test Code in this issue

node version: v6.0.0
hapi version: ^15.2.0
boom version: ^4.2.0
os: OS X El Capitán
Outside a Docker Container

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 13, 2016

I've been able to reproduce it:

"boom": "^4.2.0"
"hapi": "^15.2.0"
node v6.9.1

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 13, 2016

I can not reproduce this on

boom: 4.20
hapi: 15.2.0
node: 4.6.1

Loading

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 13, 2016

My versions are quite old (should get around to bumping them), I do see the problem on:

Boom: 3.2.2
Hapi: 13.5.3
node: 6.2.0

Loading

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 13, 2016

I noticed that the property output.headers gets populated after the first reply, an then the response is not send anymore. I made a little experiment where I set the headers back to an empty object and the response gets sent every time now.

    handler: function (request, reply) {
        if (Object.keys(ERROR_TEST.output.headers).length > 0) {
            ERROR_TEST.output.headers = {};
        }
        reply(ERROR_TEST);
    }

So I looked it up and changed this line https://github.com/hapijs/hapi/blob/master/lib/transmit.js#L117

to:

response.headers = Object.assign({}, error.headers);

So that the boom headers are copied to the response instead of using a direct reference and this worked as well. But I don't know if this is the right path to solving this problem

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 13, 2016

@sirgallifrey but why are the boom headers empty?

Loading

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 13, 2016

They start empty when I create the Boom like so

console.log(Boom.forbidden('Error Test').output.headers); // {}

The headers shouldn't be empty at this time?

These headers get populated by transmit.marshal

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 13, 2016

Loading

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 13, 2016

When I create a unauthorized boom only with a message it wont get a header, when I set it with a scheme it get the 'WWW-Authenticate' header as documented. But with both booms I still got the same behavior described by @wwalser.
Maybe Booms are not made to be reused? Maybe the correct thing would be create a new one every time? (I'm just throwing ideas here)

Loading

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 13, 2016

I got another clue, the only header that is messing things up is 'content-encoding' if I remove just him the response is sent every time. (I need to remove it every time before calling reply)

If 'content-encoding' is already set (see here) encoding will be set to null (here) unlike the successful response that got 'gzip' here, and because encoding === null this line wont be reached and the compressor will remain equal null.

this is as far as I got by now, I will continue digging

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 14, 2016

@sirgallifrey Can you share a snippet that I can run to reproduce this? So I can help actually debugging :P

Loading

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 14, 2016

I'm using the snippet @wwalser provided with:

boom: 4.2.0
hapi: 15.2.0
node: 6.9.1

if you want I can setup a docker env with node 6.9.1

'use strict';

const Boom = require('boom');
const Hapi = require('hapi');

const ERROR_TEST = Boom.forbidden('Error Test');

const server = new Hapi.Server();
server.connection({ port: 3000 });

server.route({
    method: 'GET',
    path: '/',
    handler: function (request, reply) {
        reply(ERROR_TEST);
    }
});

server.start((err) => {

    if (err) {
        throw err;
    }
    console.log(`Server running at: ${server.info.uri}`);
});

Loading

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 14, 2016

I appreciate everyone who is pitching in to help find the problem. I've done a bit of investigation:

In that example code, If just after declaring ERROR_TEST, you add Object.freeze(ERROR_TEST.output.headers);, then hit the route, you will very quickly spot the problem.

I generally work under the assumption that an object that I have created and handed to Hapi won't be mutated but I can see why it's simpler to assume it does given JavaScript's leaning toward mutability. This thinking lead me to a few places in code where I have a pattern like:

const reusableError = Boom.badRequest('foobar');
server.route({
  method: 'GET',
  path: '/',
  handler: (request, reply) => {
    if (request.something) {
      reply(reusableError)
    }
    
  }
});

It's not hard to change reusableError to:

const reusableError = () => Boom.badRequest('foobar');

It was just confusing when it initially happened and took an hour of debugging to discover that Boom was the thing causing the server to be completely silent.

The real problem was the lack of errors or warnings.

If you want to resolve this by ensuring that error objects passed into reply aren't mutated, changing #L117 to response.headers = Hoek.clone(error.headers); will do the trick. If you want to resolve this by letting people know that re-using error objects is not supported, it's a little more tricky.

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 15, 2016

@sirgallifrey I realize I already had a snippet derp :D
I don't have node 6 atm so can't really test

@wwalser it might be handy to add a test that reproduces your error so someone can swoop in and easily fix it (or you if you find what is causing this in node 6 and not 4)

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 15, 2016

I'm gonna label this as a bug since this does used to work in node 4 and feels like it still should

Loading

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 15, 2016

I am not able to reproduce the conditions under which this bug does not occur.

I've created: https://github.com/wwalser/hapi-boom-mutation

and I've tried it with:

  • node 4.2.1, 4.6.1, 6.2.0 & 7.1.0
  • hapi 13.5.0, ^13.5.0 & ^15.2.0
  • boom 3.2.1, ^3.2.1, & ^4.2.0

The bug shows itself under all of these.

I'm off to write a test now and I'll report back once that's done.

Loading

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 15, 2016

Thanks to @sirgallifrey for his investigation hints which lead me to figure out a bit more of what's happening.

Use this to see the problem:
curl -H "Accept-Encoding: gzip" -v localhost:3000

On the first request, the headers of the boom object are empty which means Hapi uses it's defaults of gzipping responses if the client has indicated that it understands gzip (all modern browsers). This is a good thing in that Hapi default to sending gzipped responses most of the time out of the box.

The downside is that along the way it mutates the Boom object's headers to contain content-encoding: gzip. On subsequent requests which use this now mutated Boom object, Hapi inspects the headers, notes the presence of a content-encoding header and decides that the programmer must have encoded the response themselves and thus doesn't touch it, it sends along both the content-encoding header and the content itself untouched.

The final result is that the second request has a content-encoding: gzip header but unencoded content. Firefox, Chrome and Safari all balk at this, Firefox and Safari are slightly more helpful in that they say exactly what the trouble is, "can't decode content."

There is a test which enforces the case where content-encoding is set by a handler: test/transmit.js#L1461. It's just getting set by mistake in this particular case.

Loading

wwalser added a commit to wwalser/hapi that referenced this issue Nov 15, 2016
wwalser added a commit to wwalser/hapi that referenced this issue Nov 15, 2016
@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 16, 2016

Test implemented:

Test branch - Test commit - Travis build

I've also implemented my proposed fix here and tested here.

Loading

wwalser added a commit to wwalser/hapi that referenced this issue Nov 16, 2016
@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Nov 16, 2016

that's great @wwalser! I have just a small opinion on your fix, so I left a comment on your commit

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 16, 2016

@wwalser 👏 nice! Can you open a PR so the maintainers can look at it and merge the fix?

Loading

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 16, 2016

@wwalser I did a quick test and I think this is also happening when replying with the same Error object twice. If you change the error and data in your test to

            const error = new Error('test');
            const data = JSON.stringify({
                statusCode: 500,
                error: "Internal Server Error",
                message: "An internal server error occurred"
            });

the test also fails.

As for the proposed fix I think @sirgallifrey is right. I think cloning the full error is more future proof and makes more sense as in that .fail does not modify the original error object (being it boom or plain error)

Loading

@Marsup
Copy link
Contributor

@Marsup Marsup commented Nov 16, 2016

Good luck cloning errors, they are full of internals, you'll probably mess up the stack or something.

Loading

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 16, 2016

The proposed change fixes the problem with reusing errors and reusing booms. We don't clone the error, Hapi wraps errors using Boom.wrap (which eventually gets to L#108) before it calls transmit.prototype.fail. Then inside of fail, we clone boom.output.

I'll add a test for reusing an error as well since this has come up twice in the discussion.

Loading

hueniverse added a commit that referenced this issue Nov 29, 2016
Ensure Boom objects can be reused - Fix for issue #3378
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse changed the title Hapi Server becomes unresponsive after second call #140 Server fails to reuse Error object Nov 29, 2016
@hueniverse hueniverse changed the title Server fails to reuse Error object Server fails to reuse Boom object Nov 29, 2016
@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
5 participants