Refactor and Optimize Match Resource Definition

[manuelgdlvh]

Current Behavior

Currently, to check if a resource matches a definition using capture_match_info_fn, we always need to pass a function to perform additional checks.

This causes two problems:

• The check function is called inside the capture_match_info_fn, so we cannot determine if fails on the check or matching part.

• If the caller doesn't want to perform any extra checks, they still have to pass a dummy closure.

Possible Solution

Instead of calling capture_match_info_fn directly and applying the guard check function (or another function passed as a parameter) inside the function, we can change the approach. Let the capture_match_info function only handle the task of resource matching using the service request and we can create a more general function wrapper where runs the matching and check.

Also, what we could do in the routing layer is execute the check function only when capture_match_info returns true (meaning the resource definition matches) and only one time.
Then we stop the flow always but returning the resource definition only if check function returns success.

This avoid looping through all resource definitions always (if route not found or found but fails in check function), but we lose the flexibility of having multiple resource definitions with same routing configuration's but different guards (this flexibility is really needed/useful? Always routes can be defined using the join of the two defined route guards).

This change improves performance and reduces resource usage in the routing layer using early return.
It also improves abstraction and responsibility separation by avoiding the need to pass dummy closures when no check is needed.

Would be a pleasure to do my first contribution!
Kind Regards,
Manuel

[robjtede]

we lose the flexibility of having multiple resource definitions with same routing configuration's but different guards

apart from the most common method routing case, this feature of the router is what allows setting up things like virtual hosting

[manuelgdlvh]

Thanks for your time @robjtede ,

What do you think about computing a flag at the push() level in the Router builder to check for path conflicts?
I think this could be useful in cases like virtual hosting or other situations where duplicate paths are allowed.
The flag could help us know when we need to loop through all the resources.

Something like this:

That can be used in the recognize_fn's.

[robjtede]

Yea I'm definitely happy to accept PRs for performance improvements including such flags. I'd just ask that this isn't a breaking behavioral change, ie. all tests in actix-web should still pass.

Could you also run the router benchmark and provide the old and new numbers? I'm curious how much impact this will have.

[manuelgdlvh]

Hi @robjtede , as you can see, with the new implementation, there is no performance degradation when the guard check fails.
I had to implement a new group test to cover the case where the patch matches but the guard check fails — that's where the new implementation maintains the same performance as the old one when the resource is found and the check is OK.
Basically, it's the same implementation as the router benchmark in Actix-Router, but using a dummy closure that returns false.

New implementation:

Old implementation:

Since this is not a breaking change, as you mentioned, all tests are passing.

I will start working on this PR to get it ready for production.

Thanks!