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

request.state occasionally null #2505

Closed
kenkouot opened this issue Apr 15, 2015 · 13 comments
Closed

request.state occasionally null #2505

kenkouot opened this issue Apr 15, 2015 · 13 comments
Assignees
Labels
Milestone

Comments

@kenkouot
Copy link

@kenkouot kenkouot commented Apr 15, 2015

Edited: We were originally experiencing this bug on 8.2.0 and now still in 8.4.0.
We noticed that a small volume of requests started throwing odd errors. We're using two plugins, crumb and hapi-i18next and those plugins will error out trying to access a property of request.state, which turns out to be null.

The volume of this error is pretty low, it affects <0.01% of requests for at a non-peak volume of 1.5k rpm. Any help would be greatly appreciated.

Sample error (note, example if with crumb but other plugins that check request.state) will also fail:

Stack trace
TypeError: Cannot read property 'crumb' of null
at generate (/usr/www/440/node_modules/crumb/lib/index.js:156:34)
at /usr/www/440/node_modules/crumb/lib/index.js:85:13
at /usr/www/440/node_modules/hapi/lib/handler.js:312:22
at iterate (/usr/www/440/node_modules/hapi/node_modules/items/lib/index.js:35:13)
at Object.exports.serial (/usr/www/440/node_modules/hapi/node_modules/items/lib/index.js:38:9)
at /usr/www/440/node_modules/hapi/lib/handler.js:307:15
at internals.Protect.run (/usr/www/440/node_modules/hapi/lib/protect.js:56:5)
at Object.exports.invoke (/usr/www/440/node_modules/hapi/lib/handler.js:305:22)
at /usr/www/440/node_modules/hapi/lib/request.js:318:32
at iterate (/usr/www/440/node_modules/hapi/node_modules/items/lib/index.js:35:13)
at done (/usr/www/440/node_modules/hapi/node_modules/items/lib/index.js:27:25)
at finish (/usr/www/440/node_modules/hapi/lib/protect.js:45:16)
at wrapped (/usr/www/440/node_modules/hapi/node_modules/hoek/lib/index.js:798:20)
at done (/usr/www/440/node_modules/hapi/node_modules/items/lib/index.js:30:25)
at Function.wrapped [as _next] (/usr/www/440/node_modules/hapi/node_modules/hoek/lib/index.js:798:20)
at Function.internals.continue (/usr/www/440/node_modules/hapi/lib/reply.js:102:10)
@kenkouot
Copy link
Author

@kenkouot kenkouot commented Apr 15, 2015

Related to: hapijs/crumb#50

@kenkouot kenkouot changed the title request.state occasionally null after upgrading 8.2.0 -> 8.4.0 request.state occasionally in 8.2.0 Apr 15, 2015
@kenkouot kenkouot changed the title request.state occasionally in 8.2.0 request.state occasionally null Apr 16, 2015
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 20, 2015

I need to know where you are calling the crumb generate() method. request.state is set after onRequest is called, and any JSONP arguments are processed. If you try to access request.state (as crumb's generate() does) in onRequest or in an extension after JSONP error, it will be null because no state has been processed at that point.

@hueniverse hueniverse self-assigned this Apr 20, 2015
@kenkouot
Copy link
Author

@kenkouot kenkouot commented Apr 20, 2015

Hey @hueniverse, thanks for the reply. We're actually letting crumb call generate automatically by omitting any value for autoGenerate in the crumb plugin options. The actual code is here: https://github.com/Wikia/mercury/blob/dev/server/app.ts#L45.

I've done some testing and specified a crumb skip function that skips crumb's flow if request.state is null, and the error simply cascades into another plugin, hapi-i18next (link to source line).

@kenkouot
Copy link
Author

@kenkouot kenkouot commented Apr 20, 2015

Both crumb and my hapi-i18next plugins utilize onPostAuth and onPreHandler extensions respectively. So you're saying those are inappropriate and causing the occasional races?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 20, 2015

Do you have state.parse set to false anywhere? The flow is pretty simple and the only way that request.state is null is when you either disable cookie parsing or look for it too soon. onPostAuth and onPreHandler come after cookie parsing and should never have an issue unless you disable it somewhere.

@kenkouot
Copy link
Author

@kenkouot kenkouot commented Apr 20, 2015

No, in our route config we have a failAction set to log for state, but we are definitely not setting state.parse to false anywhere.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 20, 2015

Any JSONP?

@kenkouot
Copy link
Author

@kenkouot kenkouot commented Apr 20, 2015

None of our route handlers implement JSONP either 😭

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 10, 2015

Is this still an issue? Any new insights?

@devinivy
Copy link
Member

@devinivy devinivy commented Sep 1, 2015

If the cookie header itself is malformed and your failAction is not 'error', you can indeed get a null state later in the request lifecycle. hapi sets request.state to whatever the statehood module hands it– here you can see null being passed-off: https://github.com/hapijs/statehood/blob/ad84fa183888dfff017fc8d6e9d0496127d7eafe/lib/index.js#L128-L132

@devinivy
Copy link
Member

@devinivy devinivy commented Sep 1, 2015

I'd just like to add that this behavior probably changed between hapi v7 and hapi v8 when statehood's ties to hapi were refactored. If it's really bugging you @kenkouot, I imagine that patching request.state in onPreAuth would be an acceptable thing to do– request.state is initialized to an empty object (that's what you'd get if there were no cookie header at all).

I'm not entirely sure if this null business is considered a bug or not, but it clearly can be confusing. I personally think it's nice to assume that request.state is an object, as the docs would indicate.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Sep 1, 2015

It is a bug. Thanks for digging into it. I'll take another look.

@hueniverse hueniverse added the bug label Sep 1, 2015
@hueniverse hueniverse added this to the 10.1.0 milestone Sep 25, 2015
@devinivy
Copy link
Member

@devinivy devinivy commented Sep 29, 2015

@nlf will bugs fixes like this make their way into hapi-lts?

@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