Add callback handler to report internal HTTP parsing errors (431, 400, 505)#1891
Add callback handler to report internal HTTP parsing errors (431, 400, 505)#1891pdeltour-syna wants to merge 3 commits intouNetworking:masterfrom
Conversation
| } | ||
|
|
||
| /* Attaches an error handler for internal HTTP parsing errors */ | ||
| TemplatedApp &&onHttpParsingError(MoveOnlyFunction<void(HttpRequest *, int, std::string_view)> &&errorHandler) { |
There was a problem hiding this comment.
Mainly this one needs a better name
There was a problem hiding this comment.
Error is a bad name. People will register it and think they need to handle it in some way. That's common for other libs. It sucks.
Log or logging could be used like App::log that simply gives you information of happenings that you don't need to handle.
There was a problem hiding this comment.
App::log could get things like
- HTTP error being returned to client
- Connection was opened
- Connection was closed
Neither of these events need handling, they just need logging, so App::log makes more sense than anything containing "error".
One of the main benefits of the uWS interface is that error handling and regular close is the same handler, meaning the same business flow. This is different from other libs that have open/error, open/close as two different flows of logic. That sucks, and it makes the app more complex than it has to be.
There was a problem hiding this comment.
I initially looked at using App:filter, but it seemed more like a socket level notification. Also the info I would like to collect on unhandled http requests is :
- request info (method / URL / queries / headers)
- status code
- nice to have - response body (detail of the status)
I agree that there could be a better naming for the callback handler. e.g. App::log (or App::logEvent ?)
I don't mind combining HTTP error / connection open / connection close into a single callback, where there could be even more events in the future (but maybe that would open the door for overpopulating this with events). On the other hand the information returned for connection open/close and HTTP error is completely different.
How could the function signature look like then ?
App::log(int eventType, HttpRequest *req, int statusCode, string_view responseBody)
where eventType:
- -1 : means 'Connection closed' (req == null / statusCode N/A / responseBody empty)
- 0 : means 'Unhandled http request' (i.e. request was not valid and produced internal error) with valid Req / statusCode and responseBody
- 1 : means 'Connection opened' (req == null / statusCode N/A / responseBody empty)
Alternatively when not combining the connection open/close with the unhandled request, the handler could be called something like : App::unhandledReq
| consumedTotal += consumed; | ||
| /* Parse query */ | ||
| const char *querySeparatorPtr = (const char *) memchr(req->headers->value.data(), '?', req->headers->value.length()); | ||
| req->querySeparator = (unsigned int) ((querySeparatorPtr ? querySeparatorPtr : req->headers->value.data() + req->headers->value.length()) - req->headers->value.data()); |
There was a problem hiding this comment.
I moved this code, because HttpRequest::getUrl() requires querySeparator to be set, the below return on error will call the new callback handler and this allows to get the URL of the failed request for these cases
|
Hi, didn't receive any feedback lately, how can we proceed with this ? |
|
Hi, I would like to stick with the official uWebSockets build, therefor I would like to have the callback handler included. Can you take a look ? |
|
Any feedback on the progress would be appreciated |
2 participants
In order to track all requests in my project, I also want to track incoming requests that produce an internal http parsing error (mainly 431 Request Header Fields Too Large). As internal parsing errors result in response being written to the socket and closing the socket, there is no notion of these requests in the application.
By adding a callback handler, the application can be informed of requests that produced an internal error, allowing for an application to keep track of these requests and keep metrics on that.