Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Added Application::reset() #32418

Merged
merged 1 commit into from Jul 8, 2019

Conversation

Projects
None yet
5 participants
@lyrixx
Copy link
Member

commented Jul 7, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Symfony leaks a lot in CLI when using the EventDispatcher. I'm working on it.

But instead of fixing the root cause, it can be legit to go quick and to reset all services.
So IMHO, this services should be exposed, and always available

@lyrixx lyrixx changed the base branch from master to 4.4 Jul 7, 2019

@lyrixx lyrixx force-pushed the lyrixx:service-resetter branch from 821bf0b to 4697907 Jul 7, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 8, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I understand, but I'd prefer not if possible, as having a non-leaking core is very important to me. Providing workarounds could lessen the urge to fix and not really help in the long run.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Would making Kernel implement ResetInterface be a cleaner alternative? (if we really need this hook)

fabpot added a commit that referenced this pull request Jul 8, 2019

bug #32421 [EventDispatcher] Add tag kernel.rest on 'debug.event_disp…
…atcher' service (lyrixx)

This PR was submitted for the 4.4 branch but it was merged into the 3.4 branch instead (closes #32421).

Discussion
----------

[EventDispatcher] Add tag kernel.rest on 'debug.event_dispatcher' service

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? |
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

In CLI, Symfony leaks because it uses the TraceableEventDispatcher.
Let's make this service resetable.

This PR is related to #32418

---

Side note: Forcing user to use the `ServicesResetter` is IMHO bad for
the DX. I prefer to have something that does not leak by default.
More over, the TraceableEventDispatcher stores some informations for the
profiler. But in CLI it does not make sens, since the tooling does not
exist. I have already fixed such issue for monolog in #30339. I think we
should do the same for the TraceableEventDispatcher.

---

Note: When using #32418 + this PR and with the following code, it does not leak **at all**

<details>
<summary>code</summary>

```php
<?php

namespace App\Command;

use Psr\Log\LoggerInterface;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\EventDispatcher\Event;
use Symfony\Component\HttpKernel\DependencyInjection\ServicesResetter;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;

class LeakCommand extends Command
{
    protected static $defaultName = 'leak';

    private $dispatcher;
    private $resetter;
    private $logger;

    public function __construct(EventDispatcherInterface $dispatcher, ServicesResetter $resetter, LoggerInterface $logger)
    {
        $this->dispatcher = $dispatcher;
        $this->resetter = $resetter;
        $this->logger = $logger;

        parent::__construct();
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $this->dispatcher->addListener(MyEvent::class, function ($e) {

        });
        for ($i=0; $i < INF; $i++) {
            $output->writeln(memory_get_usage());

            $this->dispatcher->dispatch(new MyEvent());
            $this->logger->debug('some log');

            // if ($i > 2000) {
            //     meminfo_dump(fopen('/tmp/my_dump_file.json', 'w'));
            //     die;
            // }

            usleep(100);
            $this->resetter->reset();
        }
    }
}

class MyEvent {}

```
</detail>

Commits
-------

5249eaf [EventDispatcher] Add tag kernel.rest on 'debug.event_dispatcher' service
@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

I understand, but I'd prefer not if possible, as having a non-leaking core is very important to me. Providing workarounds could lessen the urge to fix and not really help in the long run.

I totally agree but there is two things here:

  1. having a non leaking core is a must have (BTW, you need to see this comment?)
  2. be able to reset the framework to a clean state. When you are working on a long running process, you need to reset everything at some point. For example when a new AMQP message comes, you want to empty Monolog's Buffer, to clear the doctrine's Unit Of Work, etc... Because yes, there are some services that leak by nature.

Both points are really wanted. We can already wire everything in Messenger to clear everything (If not done, I did not check). I already open an issue in swarrot about that too.

But I'm also writing 100% custom worker that does many things. I need to do that by hand too :)

Would making Kernel implement ResetInterface be a cleaner alternative? (if we really need this hook)

I hesitate between Two approaches. This PR and adding Command::reset(). But it's not possible anymore since commands are not 'container aware' anymore.

Adding Kernel::reset() is a very nice idea too! I will update update the PR.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

Hmm, I updated the PR and I have a strange feeling. This means injecting the kernel. I think I never injected the kernel. It's a bit like injecting the Container to me. I'm not sure it's cleaner.

I hesitate between Two approaches. This PR and adding Command::reset(). But it's not possible anymore since commands are not 'container aware' anymore.

Actually this is possible: Command::getApplication()->getKernel()->reset() :)

So we have Many choices here:

  1. Make ServicesResetter "public": autowire + always preset
  2. Add Kernel::reset() and let people inject the kernel where needed
  3. 2 + expose Command::reset() to hide the fact you can inject the Kernel , and to make things smoother

WDYT? I would go for 1 or 3, not 2

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Add reset() on Application? Would make sense to me.

@lyrixx lyrixx force-pushed the lyrixx:service-resetter branch 2 times, most recently from 3d1a6fa to fa26c2d Jul 8, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

Here we go :)

@lyrixx lyrixx changed the title [FrameworkBundle] Add support for autowiring ServicesResetter and make it always available [HttpKernel+FrameworkBundle] Added Kernel::reset() and Console\Application::reset() Jul 8, 2019

@lyrixx lyrixx force-pushed the lyrixx:service-resetter branch from fa26c2d to e1afb01 Jul 8, 2019

@lyrixx lyrixx changed the title [HttpKernel+FrameworkBundle] Added Kernel::reset() and Console\Application::reset() [Console] Added Application::reset() Jul 8, 2019

@lyrixx lyrixx force-pushed the lyrixx:service-resetter branch from e1afb01 to 15ba579 Jul 8, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@nicolas-grekas Here we go 👍 I addressed your comments

@nicolas-grekas
Copy link
Member

left a comment

lean, nice :)

@fabpot

fabpot approved these changes Jul 8, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit 15ba579 into symfony:4.4 Jul 8, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 8, 2019

feature #32418 [Console] Added Application::reset() (lyrixx)
This PR was merged into the 4.4 branch.

Discussion
----------

[Console] Added Application::reset()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Symfony leaks a lot in CLI when using the EventDispatcher. I'm working on it.

But instead of fixing the root cause, it can be legit to go quick and to reset all services.
So IMHO, this services should be exposed, and always available

Commits
-------

15ba579 [Console] Added Application::reset()

@lyrixx lyrixx deleted the lyrixx:service-resetter branch Jul 8, 2019

@lyrixx lyrixx referenced this pull request Jul 8, 2019

Open

Leverage ResetInterface #193

/**
* {@inheritdoc}
*/
public function reset()

This comment has been minimized.

Copy link
@Tobion

Tobion Jul 11, 2019

Member

what's the point of an empty reset method? why not implement reset only in the frameworkbundle application?

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 11, 2019

Author Member

Command::getApplication() returns this very class according to the phpdoc.
With this, phpstan is happy

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 11, 2019

Member

Please, please, please, stop with the phpstan mania. We don't care if phpstan is happy or not. This is just a tool.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 11, 2019

Author Member

Actually I did it to make @nicolas-grekas happy (see previouses comment) he is not a tools 😅😁

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 11, 2019

Member

The reason @Tobion is that this method is now part of the semantics of the API exposed by the base Application class. It's a template method (from the template design pattern).

This comment has been minimized.

Copy link
@Tobion

Tobion Jul 11, 2019

Member

template pattern does not apply here. template pattern methods must either be abstract (to force subclasses to implement them) or function as helper method hooks (with can have an empty body). neither is the case here. it's just an empty method providing no functionality. but it's not that important to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.