Weird workflow on every request

[vltr]

After spending some time with the Sanic codebase, I feel like there's a weird / awkward workflow related to every request received by the server - please, correct me if I'm wrong. I will use the keywords server to the HttpProtocol, app to Sanic, request to Request, router to the Router and handler to whatever instance will handle the request.

1. After receiving a connection, the server starts processing the data to parse the headers, create the request, feed the body (if any) and so on. Inside the server, it calls the router to get the handler and check if the handler is a stream;
2. When all data is received (if not a stream), it then calls the app to handle the request;
3. The app then calls the router again to get the handler again and thus process the request if there's a handler.

The first problem that I see here is that the router may be called twice for a single request (and I'm not ignoring that there's an lrucache decorating the Router._get method).

The second problem with that is why do we need to process the whole incoming data, create a request instance and then see if there's a handler for it, while we could do that earlier and stop processing (probable useless) data if no handler is found?

One possible solution would be to move this server-router-app-router logic to the server, storing the handler on it as soon as possible and if everything is good to go, call the handler. It should not have any impact to the current API.

I already made PR #1310 that's kind of related to some "inter-dependency" of specific designed variables under the hood and there should not be a problem for me to work on the subject of this issue 😉

[sjsadowski]

This is probably related to creep over time as the handler evolved. It wouldn't hurt to do as you describe, which to paraphrase would be move the logic up in the request process and bail out if there is no handler.

[vltr]

@sjsadowski yes, and I found another call to the same router method inside the Request itself, in an uncached property. It does looks like a patchwork that needs some heartwarming attention.

[ahopkins]

Might I suggest this be something we schedule for after the December release?

[sjsadowski]

It seems like a minor change (ha ha, seems like... ha... how many times that has done me wrong) but it could be a fairly big performance boost if we're not waiting for the entire request to validate the handler exists. It could still be valid for the December target because it's not a feature request, imho, it is refactoring and tightening things up. Also it seems kind of like a bug.

[vltr]

Yeah, well, I'm biased here because I believe it is not a minor change; it will probably have some impact in other protocols (websockets). I also believe it will give a marginal performance boost but I really can't measure the impact of this change (although it is something I want to see on Sanic ...)

[sjsadowski]

Ok, so let's shoot for post-December based on any other feedback we get, unless there's a compelling reason to push it in.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.