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

Ensure Boom objects can be reused - Fix for issue #3378 #3380

Merged
merged 4 commits into from Nov 29, 2016

Conversation

@wwalser
Copy link
Contributor

@wwalser wwalser commented Nov 16, 2016

Fix for #3378 Let me know when it's all clear and I'll squash.

Cheers,
Wes

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 16, 2016

Added an additional regression test based on comments in the issue's discussion thread, see #3378.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 17, 2016

I think this is correct unless @hueniverse knows of a better fix.
Only thing I worry about might be perf with all the cloning

@kanongil
Copy link
Contributor

@kanongil kanongil commented Nov 17, 2016

I'm confused – why should this be supported? Boom objects are not designed for reuse.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Nov 17, 2016

Same for me, I feel that it's a bad pattern. Pre-generating errors won't improve your performance by any means (if that's the intention), and you'll lose a meaningful way of debugging, that is stacktraces. I'd favor error factories for that.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 17, 2016

I can agree on that, I wouldn't do it either but the way hapi handles it now does not feel correct either

@devinivy
Copy link
Member

@devinivy devinivy commented Nov 17, 2016

I agree with both sides– ideally I'd like to see hapi deal with this better, but booms are actual Errors, which shouldn't be reused.

@arb
Copy link
Contributor

@arb arb commented Nov 17, 2016

clone also isn't cheap and should only be used as a last ditch.

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 17, 2016

Thanks for everyone's input. Using Hapi for the last eight months has been an absolute pleasure and I haven't contributed to a project in recent memory whose community is this active. For those confused, reading the original bug discussion may aid in better understanding. To be clear for my own part, I wasn't reusing Booms in an effort to be more performant. I did it because it felt like cleaner code to have constants representing the errors that can occur above the handler. Especially in cases of plugins where affording for other developers scanning the code for meaning is important. Additionally, and I know this is unlikely to sway anyone, I would never expect a tool/framework to which I hand something to mutate that thing. It's my data, I've just given it to Hapi in order to create IO.

you'll lose a meaningful way of debugging

That's a compelling argument for Error's. I was reusing Boom objects. The fact that it happens to support proper Errors as well was mostly an accident.

I was concerned about performance as well but having read Boom more carefully I think the .output objects are so shallow and simple that it shouldn't be a problem.

Here is an example of common content of .output at the time that we clone it based on Boom's implementation of .wrap and .initialize:

    error.output = {
        statusCode: 500,
        payload: {
            message: 'An internal server error occurred'
        },
        headers: {}
    };

Again, it's not cloning the entire boom or error object. Only .output which is created by Boom.

It's cool with me if we don't support re-use it though. If we go another route through, I would suggest figuring out a way of providing some sort or error or warning to the user since the side effect of reuse currently is that the response appears empty in several commonly used clients (Chrome and curl are the ones I've been testing with).

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 17, 2016

What is the reason the headers are copied? For errors like unauthorized right?
If so another solution could be to handle the special content-encoding case.

I do agree that hapi should not modify (boom) errors you give it.

@devinivy
Copy link
Member

@devinivy devinivy commented Nov 17, 2016

I'm not first and foremost concerned with performance here. The reason I'm not into supporting reuse is that boom is an Error, and thus holds information like boomErr.stack that is absolutely particular to moment that the error occurs. Errors in general aren't good candidates for reuse. If you want reusable const-style errors, consider adding a reply decoration that accepts a reusable error code.

At the same time, I do see how hapi hanging upon error reuse is very confusing and isn't ideal.

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 17, 2016

What is the reason the headers are copied?

Headers are added by two of Boom's errors, unauthorized and methodNotAllowed.

At the same time, I do see how hapi hanging upon error reuse is very confusing and isn't ideal.

Hapi doesn't actually hang. It responds with headers that don't match the content causing clients to display no output because the response body fails to decode. See my comment in the original thread.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 17, 2016

@wwalser I meant that by 'hanging up' ^^

I think we all agree that the use case is an anti pattern that you should not be doing but is this a bug then or not?
It feels like hapi should play nice though :/

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 17, 2016

A few thoughts re: is this a bug?

  1. Hapi's behavior of respecting an error's content-encoding and assuming the API user has already encoded their content thus returning a content body that can't be decoded is not a bug. Indeed, this is important in order to allow API users the ability to provide custom content encodings. However, because of how confusing this case can be, I would love to have an error, warning or some indication that this is what is happening. Multiple people opened an issue believing this to be a bug and it took 3 people reading through the call path to uncover what was happening and why.
  2. Reusing Boom's is and anti pattern. I feel compelled to point out that it is only an anti-pattern because the Error is instantiated upon creation and not on use. This seems like the real anti-pattern to me. It is indeed very easy to use const userNotFound = ()=>Boom.unauthorized('…', '…'); instead of const userNotFound = Boom.unauthorized('…', '…').
  3. The fact that mutation occurs still bothers me but I respect the fact that I lean toward immutability and purity whereas most of the JS community does not.

I feel that prescribing what should be done would be to step out of bounds as someone who has just shown up. I defer to those who have cared for and crafted Hapi for much longer than myself. Thanks for looking into the issue and PR :).

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 18, 2016

1: agree
2: this is just how js errors work?
3: I would also prefer that hapi did not modify the error
4: As a hapi user you are very much entitled to an opinion, that is why we have this PR system :P But I also don't feel like I'm experienced enough to call this one :D

@wwalser
Copy link
Contributor Author

@wwalser wwalser commented Nov 18, 2016

2: this is just how js errors work?

I only meant that there's no need for Boom to create it's internal Error until the Boom is actually being handled by Hapi. If it instead created the Error at the time of use instead of instantiation the stack traces would be correct regardless of the way that the Booms are created, referenced, copied etc.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 18, 2016

Well since Boom errors are actual errors but with more info I think that would be weird but I see where you come from

@hueniverse hueniverse self-assigned this Nov 29, 2016
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Nov 29, 2016

  1. You should not reuse errors.
  2. hapi should not mess around with user responses, errors or otherwise.

This is not the right solution, but the tests are useful.

@hueniverse hueniverse merged commit 4596dd9 into hapijs:master Nov 29, 2016
2 checks passed
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

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

Successfully merging this pull request may close these issues.

None yet

8 participants