Baldinof / roadrunner-bundle

A RoadRunner worker integrated in your Symfony app
MIT License
255 stars 46 forks source link

Reset symfony services on every handled request #115

Closed rmikalkenas closed 1 year ago

rmikalkenas commented 1 year ago

Add support for resetting container services after each handled request.

Without this patch, services are not resetted. For example Monolog's Finger Cross handler is never reset nor flushed. So if the first request triggers an "error" level message, all others message will log and overflow the buffer.

Even symfony's messenger component has such implementation for long running process: https://github.com/symfony/messenger/blob/6.2/EventListener/ResetServicesListener.php

Baldinof commented 1 year ago

Hello,

I tried on a fresh project and the service resetter is called before each request, and finger crossed handler is working fine.

What version of this package and Symfony are you using?

rmikalkenas commented 1 year ago

@Baldinof latest version of bundle and Symfony 6.2

bundle's configuration:

baldinof_road_runner:
    kernel_reboot:
        strategy: always
    metrics:
        enabled: false

rr config is simplified to 1 worker and max_jobs: 100

Just double checked: Symfony\Component\HttpKernel\DependencyInjection\ServicesResetter does no get called between requests. Also I cannot find a code which would call it. Kernels reset method does not call it, neither there are any listeners which would do it. Could you share your setup?

FluffyDiscord commented 1 year ago

Yes the service resetter is not being called. In prod its being skipped completely and I have to use custom middleware to do so

Baldinof commented 1 year ago

The service resetter is called here: https://github.com/symfony/symfony/blob/4813d669ff27cc0d1c58a69af9c781b68d25c052/src/Symfony/Component/HttpKernel/Kernel.php#L114

As you are using kernel_reboot.strategy: always, the container is re-created and services do not need to be resetted: the container is fully destroyed / re-created on each request and you always get fresh services.

If you can I recommend you to not use the always strategy to get the best of RoadRunner.

rmikalkenas commented 1 year ago

The service resetter is called here: https://github.com/symfony/symfony/blob/4813d669ff27cc0d1c58a69af9c781b68d25c052/src/Symfony/Component/HttpKernel/Kernel.php#L114

That's not true, because reboot method explicitly defines $this->resetServices as false, resulting to not call resetter

As you are using kernel_reboot.strategy: always, the container is re-created and services do not need to be resetted: the container is fully destroyed / re-created on each request and you always get fresh services.

If you can I recommend you to not use the always strategy to get the best of RoadRunner.

Agree with always strategy. But what about others or custom one? This means that it's not safe to use them

I would strongly suggest to reconsider the idea. If you are not happy with the fact that Symfony services resseter is being used - could we then at least implement the new event to have an ability to create a listener which would be called right after successfully consumed response?

Baldinof commented 1 year ago

I just tried on a fresh project and service resetter is called.

The Symfony code is not really straightforward:

I validated this on a very fresh project:

Good things to know

Symfony will reset only services that has been initialized during the request. So if you need something to be run after all requests, you need a middleware, or to listen the kernel.terminate event.

A class implementing Symfony\Contracts\Service\ResetInterface is not meant to reset other services, it should be used if your service need to be resetted after it has been used.

When using kernel_reboot.strategy: always it is normal that ther service resetter is never called, because services are never re-used.

With the latest version of RR (2023.x), it seems having http.pool.debug: true creates a fresh php process on each request. So if you are using the .rr.dev.yaml config file, it is also normal to not see service reset calls, see here

Agree with always strategy. But what about others or custom one? This means that it's not safe to use them

I am not sure to understand your comment, all strategies are safe to use. It's just that when using the always strategy, you don't benefit from re-using the kernel AND the Symfony container, so performance gain will not be as good as with the default strategy.

If you can provide me a working reproducer that shows service resetter is not being called, I would be glad to dig why.

rmikalkenas commented 1 year ago

@Baldinof you are correct. Appreciate for a very clear explanation. Tried all the cases and everything works as expected. Closing this PR as resolved

Baldinof commented 1 year ago

Cool :)