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

[Monolog] Disable DebugLogger in CLI #30339

Merged
merged 1 commit into from Mar 17, 2019
Merged

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Feb 22, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass?
Fixed tickets #30333 symfony/monolog-bundle#165 #25876
License MIT
Doc PR
Considering this code:
namespace App\Command;

use Psr\Log\LoggerInterface;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

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

    private $logger;

    public function __construct(LoggerInterface $logger)
    {
        $this->logger = $logger;

        parent::__construct();
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $reportedAt = time();
        while (true) {
            $this->logger->info('Hello');

            if (time() - $reportedAt >= 1) {
                $output->writeln(sprintf('%dMb', memory_get_usage() / 1024 / 1024));
                $reportedAt = time();
            }
        }
    }
}

Without the patch

>…/dev/labs/symfony/website-skeleton(monolog %) bin/console leak
7Mb
28Mb
51Mb
76Mb
97Mb

With the patch

>…/dev/labs/symfony/website-skeleton(monolog %) bin/console leak
6Mb
6Mb
6Mb
6Mb

@nicolas-grekas
Copy link
Member

Would calling reset() periodically in your worker's main loop work? $logger->reset() or $logger->getDebugLogger()->reset()?

@lyrixx
Copy link
Member Author

lyrixx commented Feb 24, 2019

@nicolas-grekas This is another solution but A/ it's not buildin B/ it's useless to have this collector anyway
That's why I prefer this way of doing thing

@nicolas-grekas
Copy link
Member

Related to #25876

@lyrixx
Copy link
Member Author

lyrixx commented Feb 28, 2019

@nicolas-grekas Let's merge it ?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 16, 2019

@stof Are you OK with this one now?

@fabpot
Copy link
Member

fabpot commented Mar 17, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit 17533da into symfony:master Mar 17, 2019
fabpot added a commit that referenced this pull request Mar 17, 2019
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Monolog] Disable DebugLogger in CLI

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   |
| Fixed tickets | #30333 symfony/monolog-bundle#165 #25876
| License       | MIT
| Doc PR        |

<details>
<summary>Considering this code:</summary>

```php
namespace App\Command;

use Psr\Log\LoggerInterface;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

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

    private $logger;

    public function __construct(LoggerInterface $logger)
    {
        $this->logger = $logger;

        parent::__construct();
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $reportedAt = time();
        while (true) {
            $this->logger->info('Hello');

            if (time() - $reportedAt >= 1) {
                $output->writeln(sprintf('%dMb', memory_get_usage() / 1024 / 1024));
                $reportedAt = time();
            }
        }
    }
}
```

</details>

Without the patch
```
>…/dev/labs/symfony/website-skeleton(monolog %) bin/console leak
7Mb
28Mb
51Mb
76Mb
97Mb
````

With the patch
```
>…/dev/labs/symfony/website-skeleton(monolog %) bin/console leak
6Mb
6Mb
6Mb
6Mb
```

Commits
-------

17533da [Monolog] Disable DebugLogger in CLI
@lyrixx lyrixx deleted the monolog-cli branch March 17, 2019 10:36
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
fabpot added a commit that referenced this pull request Jul 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 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
@@ -75,6 +75,21 @@ public function reset()
$this->clear();
}

public function removeDebugLogger()
Copy link
Member

Choose a reason for hiding this comment

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

should be marked as internal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants