Problem/Motivation

Through #3392147: Add an allowed IP list to the Ban module, we are now injecting the config factory in the middleware. This will probably have a high performance impact, since the config factory has a lot of dependencies including the event dispatcher and the module handler.

Steps to reproduce

Proposed resolution

Only initiate the config manager when needed. We can try fixing it similiar to how it was done in #3551605: Injecting ConfigFactory into BanIpManager is expensive, use a closure

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ban-3579646

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

grevil created an issue. See original summary.

grevil opened merge request !19

grevil’s picture

grevil commented
Status: Active » Needs review

Not quite sure, if this provides any benefits, since we always call handle anyway. The closure config factory service only gets initiated once first called, but since we always call handle and the ip is assumably always set, I doubt this brings many benefits, but I might be wrong, since I never used symfony service closures before.

See https://symfony.com/doc/current/service_container/service_closures.html for further informations on the topic.

grevil’s picture

grevil commented
Assigned: Unassigned » mstrelan

Alright, please review!

grevil’s picture

grevil commented

Ran into #3579658: Using symfony service closures causes fatal error, while implementing this, but shouldn't be our concern.

mstrelan’s picture

mstrelan commented

It looks good, only one small thing. I think the deprecation message still needs to be in the constructor while the Drupal::service call should stay where it is.

grevil’s picture

grevil commented
Assigned: mstrelan » Unassigned

Alright, that should do the trick then!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Everything addressed. RTBC.

  • anybody committed 81664ab7 on 1.1.x authored by grevil 
    Resolve #3579646 "Injecting the config factory causes performance...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> I doubt this brings many benefits, but I might be wrong, since I never used symfony service closures before.

You're not wrong. This doesn't help. It only helps in auto_unban because config there is only needed when banning an IP, not when checking it.

A partial improvement is to move the allowed list check inside isBanned(), then it's only loaded on if the IP is banned, where performance possibly isn't crucial. Until you get hammered by a bot and then performance and the point if blocking in a middleware in the first place is important again.

See my comments in #3578480: Worth caching allowed IP list in middleware?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Closed (outdated)

Closing this outdated as we will resolve this in #3582177: Rewrite IP allowlist using a settings.php setting as discussed in #3578480: Worth caching allowed IP list in middleware? and #3582176: Add a performance test

Thank you @berdir!!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.