Fix after_request_complete getting nil env #179
Conversation
stevenzelinli
left a comment
stevenzelinli
left a comment
There was a problem hiding this comment.
Don't have a ton of context in this area but this change and the explanation make sense to me 👍
lib/pitchfork/http_server.rb
Outdated
| rescue => application_error | ||
| handle_error(client, application_error) | ||
| env | ||
| env || @request.env |
There was a problem hiding this comment.
| env || @request.env | |
| env || @request&.env |
If Pitchfork::HttpParser.new raises [an application_error], better to fall back to a nil than an error
There was a problem hiding this comment.
Updated to do || {} where the callback is called as we discussed 👍
hmcguire-shopify
force-pushed
the
hm-zkqwwukmtslyukwx
branch
from
e2ad959 to
1aabd21
Compare
| request_env = process_client(client, worker, prepare_timeout(worker)) | ||
| worker.increment_requests_count | ||
| @after_request_complete&.call(self, worker, request_env) | ||
| @after_request_complete&.call(self, worker, request_env || {}) |
There was a problem hiding this comment.
Should we just skip the callback instead?
I expect many callback would fail on an empty hash about as much as on nil, because some mandatory keys (e.g. PATH_INFO would be missing).
Technically, if we failed to parse the request, there's no "after request" stage.
There was a problem hiding this comment.
Yeah, that's fair.
IMO it's probably worth doing the @request&.env "saving attempt" -- because malformed or not, we did receive a request, so the callback-author probably wants to know about it. If we've failed parsing the request it's clearly not going to be complete, but there's still a decent chance it has a useful amount of context.
There was a problem hiding this comment.
Noting things we use it for:
worker_requests_countmetric- OOBGC
- OOMKiller
Since the worker's request_count is incremented on the line above I feel like it makes sense to continue allowing applications to emit that (although we could also make that conditional on the request parsing). The other two things seem less important for a request that fails to parse
hmcguire-shopify
force-pushed
the
hm-zkqwwukmtslyukwx
branch
2 times, most recently
from
0bc94ea to
4701b38
Compare
Previously, when a request caused `@request.read(client)` to raise, `after_request_complete` callbacks would end up with a `nil` `env` (which can cause `NoMethodError`s when assuming its always a `Hash`). This commit fixes the `NoMethodError` possibility by ensuring the `env` returned is at least a `Hash` so that `[]` won't raise. Note that it still can't be treated as a _valid_ Rack environment since the request wasn't able to finish parsing, but a `Hash` here is better than `nil`.
hmcguire-shopify
force-pushed
the
hm-zkqwwukmtslyukwx
branch
from
4701b38 to
53175e9
Compare
5 participants
Previously, when a request caused
@request.read(client)to raise,after_request_completecallbacks would end up with anilenv(which can causeNoMethodErrors when assuming its always aHash).This commit fixes the
NoMethodErrorpossibility by ensuring theenvreturned is at least aHashso that[]won't raise. Note that it still can't be treated as a valid Rack environment since the request wasn't able to finish parsing, but aHashhere is better thannil.