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

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

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 3 commits intosymfony:3.4fromlyrixx:event-dispatcher-reset
Jul 8, 2019

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedJul 7, 2019
edited
Loading

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

code
<?phpnamespaceApp\Command;usePsr\Log\LoggerInterface;useSymfony\Component\Console\Command\Command;useSymfony\Component\Console\Input\InputArgument;useSymfony\Component\Console\Input\InputInterface;useSymfony\Component\Console\Input\InputOption;useSymfony\Component\Console\Output\OutputInterface;useSymfony\Component\Console\Style\SymfonyStyle;useSymfony\Component\EventDispatcher\Event;useSymfony\Component\HttpKernel\DependencyInjection\ServicesResetter;useSymfony\Contracts\EventDispatcher\EventDispatcherInterface;class LeakCommandextends Command{protectedstatic$defaultName ='leak';private$dispatcher;private$resetter;private$logger;publicfunction__construct(EventDispatcherInterface$dispatcher,ServicesResetter$resetter,LoggerInterface$logger)    {$this->dispatcher =$dispatcher;$this->resetter =$resetter;$this->logger =$logger;parent::__construct();    }protectedfunctionexecute(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(newMyEvent());$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 {}

Simperfitand others added2 commitsJuly 8, 2019 07:40
This PR was merged into the 3.4 branch.Discussion----------[Console] Update inherit and add licence| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | not needed <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply   (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->seehttps://github.com/symfony/symfony/pull/32318/files#r299561556 it needs to be updated in the lowest branch.Commits-------ff0c141 [Console] Update to inherit and add licence
@nicolas-grekasnicolas-grekas added this to thenext milestoneJul 8, 2019
@fabpot
Copy link
Member

Let's merge in 3.4. Leaking memory is a bug.

@fabpotfabpot changed the base branch from4.4 to3.4July 8, 2019 06:06
@fabpotfabpotforce-pushed theevent-dispatcher-reset branch from0814b07 to5249eafCompareJuly 8, 2019 06:06
@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit5249eaf intosymfony:3.4Jul 8, 2019
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
@lyrixxlyrixx deleted the event-dispatcher-reset branchJuly 8, 2019 07:21
@lyrixx
Copy link
MemberAuthor

What do you think about my comment to remove it in cli env ?

@fabpot
Copy link
Member

@lyrixx I think it makes sense.

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

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@lyrixx@fabpot@nicolas-grekas@carsonbot@Simperfit

[8]ページ先頭

©2009-2025 Movatter.jp