Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJul 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 commentedJul 8, 2019
Would making Kernel implement |
…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 commentedJul 8, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I totally agree but there is two things here:
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 :)
I hesitate between Two approaches. This PR and adding Adding |
lyrixx commentedJul 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.
Actually this is possible: So we have Many choices here:
WDYT? I would go for 1 or 3, not 2 |
nicolas-grekas commentedJul 8, 2019
Add reset() on Application? Would make sense to me. |
3d1a6fa tofa26c2dComparelyrixx commentedJul 8, 2019
Here we go :) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lyrixx commentedJul 8, 2019
@nicolas-grekas Here we go 👍 I addressed your comments |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lean, nice :)
fabpot commentedJul 8, 2019
Thank you@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 availableCommits-------15ba579 [Console] Added Application::reset()
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| publicfunctionreset() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅😁
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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