Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4fromlyrixx:service-resetter
Jul 8, 2019

Conversation

@lyrixx
Copy link
Member

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
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

@nicolas-grekas
Copy link
Member

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
Copy link
Member

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

fabpot added a commit that referenced this pull requestJul 8, 2019
…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 forthe DX. I prefer to have something that does not leak by default.More over, the TraceableEventDispatcher stores some informations for theprofiler. But in CLI it does not make sens, since the tooling does notexist. I have already fixed such issue for monolog in#30339. I think weshould 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<?phpnamespace 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
Copy link
MemberAuthor

lyrixx commentedJul 8, 2019
edited
Loading

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 tosee 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 inswarrot 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 implementResetInterface be a cleaner alternative? (if we really need this hook)

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

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

@lyrixx
Copy link
MemberAuthor

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 addingCommand::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 + exposeCommand::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
Copy link
Member

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

lyrixx reacted with thumbs up emoji

@lyrixxlyrixxforce-pushed theservice-resetter branch 2 times, most recently from3d1a6fa tofa26c2dCompareJuly 8, 2019 09:22
@lyrixx
Copy link
MemberAuthor

Here we go :)

@lyrixxlyrixx 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
@lyrixxlyrixx changed the title[HttpKernel+FrameworkBundle] Added Kernel::reset() and Console\Application::reset()[Console] Added Application::reset()Jul 8, 2019
@lyrixx
Copy link
MemberAuthor

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

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

lean, nice :)

@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit15ba579 intosymfony:4.4Jul 8, 2019
fabpot added a commit that referenced this pull requestJul 8, 2019
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 availableCommits-------15ba579 [Console] Added Application::reset()
@lyrixxlyrixx deleted the service-resetter branchJuly 8, 2019 10:19
/**
* {@inheritdoc}
*/
publicfunctionreset()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

nicolas-grekas and ogizanagi reacted with laugh emoji

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

@TobionTobionJul 11, 2019
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

vudaltsov reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@lyrixx@nicolas-grekas@fabpot@Tobion@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp