Add callback handler to report internal HTTP parsing errors (431, 400, 505)

[pdeltour-syna]

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.

[uNetworkingAB]

Overall I like it, this was how I wanted it eventually.

You have moved this section for no reason:

            /* 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());

and I don't like the onHttpParserError naming as it implies generic "errors" in parsing (those are not exposed to the app and not expected to be handled by the user).

Something more specific is needed in naming. I will have a thinker

[uNetworkingAB]

Mainly this one needs a better name

[uNetworkingAB]

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.

[uNetworkingAB]

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.

[pdeltour-syna]

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

[pdeltour-syna]

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

[pdeltour-syna]

Hi, didn't receive any feedback lately, how can we proceed with this ?

[pdeltour-syna]

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 ?

[pdeltour-syna]

Any feedback on the progress would be appreciated

[uNetworkingAB]

I work in bursts at times of interest, not 9-5 every day. This log callback needs thinking and tests (emitting new events is risky for things like closing the socket inside the event, unsubscribing inside the event, etc.). Also it needs thinking regarding other events for logging. It also misses some basic optimization ideas like why perform if statement when you can just set the handler by default, etc. I don't like the switch I don't like the interface, it's low prio, etc.