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] add console.alarm event #47920

Closed
dkarlovi opened this issue Oct 19, 2022 · 8 comments · Fixed by #53533
Closed

[Console] add console.alarm event #47920

dkarlovi opened this issue Oct 19, 2022 · 8 comments · Fixed by #53533

Comments

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 19, 2022

Description

Currently, the Doctrine connection can get pinged on each new message if the middleware is enabled. But, the message handlers can be very heavy and timeouts short (for example, default wait_timeout on Azure MySQL is 180s).

If you have a very heavy handler, the handler execution process might be longer than the idle timeout and, once the handler completes, ack() can throw an MySQL has gone away error even though the connection was pinged successfully when the message was received.

The suggestion is to basically incorporate this https://twitter.com/lyrixx/status/1575127719624544258 into DoctrinePingConnectionMiddleware (or in a more appropriate place).

/cc @lyrixx

Example

  1. new message
  2. ping connection, valid
  3. run handler, it does a bunch of stuff (say, 3rd party APIs, etc)
  4. 5min later...
  5. ack() fails with MySQL has gone away, SSL: Connection reset by peer or similar error

This change would ensure the connection is "exercised" during the handler execution window too.

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Nov 2, 2022

If anybody is interested, I've solved this in one of our projects by decorating the messenger:consume command, it seems to work as expected.

@xabbuh xabbuh added the Feature label Nov 4, 2022
@lyrixx
Copy link
Member

lyrixx commented Nov 9, 2022

It's complicated because the signal might be already handled by another part of the code. We have stuff to do that in the console component.

Anyway, one can still remove the behavior later 😬. But in that case we can say it the user responsibility to take care of decorating the current behavior.

Anyway, this works like a charm, so I'm +1 with the feature

@dkarlovi
Copy link
Contributor Author

Thinking about this recently, it might make sense to actually add it to Symfony Console as a console event. This would mean Console itself can support this and dispatch some sort of console.tick event, the various bundles can then implement a listener which listens to that event and does whatever the appropriate action is.

Functionally it would be similar to console.signal, meaning Console itself would implement it, not the individual commands.

@dkarlovi
Copy link
Contributor Author

Discussion with @stof on slack

Prototype:

<?php
declare(ticks=1);

function tick()
{
    echo "Tick!\n";

    pcntl_alarm(1);
}

function work()
{
    echo "Sleeping 10sec...\n";
    $left = sleep(10);
    while ($left > 0) {
        $left = sleep($left);
    }
    echo "Done\n";
}

pcntl_signal(SIGALRM, 'tick');
pcntl_alarm(1);

work(); // this is the Console command running

Output

 php tick.php 
Sleeping 10sec...
Tick!
Tick!
Tick!
Tick!
Tick!
Tick!
Tick!
Tick!
Done

It seems declare(ticks=1); needs to be in the entrypoint file because if it doesn't seem to work if it's only in the file being included, so bin/console would need to define it AFAIK (needs research).

@dkarlovi dkarlovi changed the title [Messenger] add a time-based ping to DoctrinePingConnection [Console] add console.tick event May 10, 2023
@lyrixx
Copy link
Member

lyrixx commented May 12, 2023

Ticks is highly CPU intensive. And the tick declaration must be set in each PHP file where you need a tick since PHP 7 (IIRC).

I wrote a Stream Wrapper to add the declare in each PHP file.

So I don't think relying on the ticks is a good idea. But if you manage to do a POC, I'll be happy to have a look.

@dkarlovi
Copy link
Contributor Author

@lyrixx OK, so maybe we still use your alarm approach mentioned above, the point is implementing a time-based event which can be subscribed to by bundles and run their own keep-alives. Either alarm or tick or whatever works here IMO, the tick implementation was just a discussion with @stof to prove the control doesn't need to come come back to Application (from Command) to dispatch the event.

@dkarlovi
Copy link
Contributor Author

dkarlovi commented May 31, 2023

Another use case is providing Prometheus style metrics from long running commands into the log, those are important to be provided at regular intervals and not "when there's time".

@lyrixx
Copy link
Member

lyrixx commented Jul 12, 2023

IMHO, the best way is the alarm approach

@dkarlovi dkarlovi changed the title [Console] add console.tick event [Console] add console.alarm event Jul 12, 2023
@fabpot fabpot closed this as completed Oct 6, 2024
fabpot added a commit that referenced this issue Oct 6, 2024
…`ConsoleAlarmEvent` (HypeMC)

This PR was merged into the 7.2 branch.

Discussion
----------

[Console] Add ability to schedule alarm signals and a `ConsoleAlarmEvent`

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #47920
| License       | MIT

Part of #53508

This PR introduces the ability to schedule alarm signals and a `console.alarm` event. A command using this feature will automatically dispatch an alarm at the specified interval:

```php
#[AsCommand('app:alarm')]
class AlarmCommand extends Command implements SignalableCommandInterface
{
    private bool $run = true;
    private int $count = 0;
    private OutputInterface $output;

    protected function initialize(InputInterface $input, OutputInterface $output): void
    {
        $this->getApplication()->setAlarmInterval(10);
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $this->output = $output;

        while ($this->run) {
            $this->count++;
            $output->writeln('execute '.$this->count);
            sleep(1);
        }

        $output->writeln('Done');

        return Command::SUCCESS;
    }

    public function getSubscribedSignals(): array
    {
        return [\SIGINT, \SIGALRM];
    }

    public function handleSignal(int $signal, false|int $previousExitCode = 0): int|false
    {
        if (\SIGINT === $signal) {
            $this->run = false;
            $this->output->writeln('Bye');
        } elseif (\SIGALRM === $signal) {
            $this->count = 0;
            $this->output->writeln('handleAlarm');
        }

        return false;
    }
}
```

The `console.alarm` event is dispatched on every `SIGALRM` signal:

```php
#[AsEventListener(event: ConsoleAlarmEvent::class)]
final class ConsoleAlarmListener
{
    public function __invoke(ConsoleAlarmEvent $event): void
    {
        $event->getOutput()->writeln('ConsoleAlarmListener');
    }
}
```

Commits
-------

97e4391 [Console] Add ability to schedule alarm signals and a `console.alarm` event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants