Skip to content

Register amqp/worker commands as services#12

Merged
lyrixx merged 1 commit intolyrixx:amqp-workerfrom
chalasr:amqp-worker-commands
Jul 6, 2017
Merged

Register amqp/worker commands as services#12
lyrixx merged 1 commit intolyrixx:amqp-workerfrom
chalasr:amqp-worker-commands

Conversation

@chalasr
Copy link
Copy Markdown

@chalasr chalasr commented Jul 2, 2017 •
edited
Loading

  • Moved commands to their respective components
  • Registered them as services in the framework instead of container aware
  • Added more output
  • Improved output style
  • Replaced parameters by direct injection
  • Made use of Command::setProcessTitle()

@chalasr chalasr force-pushed the amqp-worker-commands branch 2 times, most recently from 0dc1713 to 452aa6a Compare July 3, 2017 08:55
lyrixx
lyrixx reviewed Jul 4, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/amqp.xml Outdated
</argument>
</service>

<service id="worker.command.list" class="Symfony\Component\Worker\Command\WorkerListCommand">
Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 4, 2017

Choose a reason for hiding this comment

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

This service is already defined in the worker.xml file

src/Symfony/Bundle/FrameworkBundle/Resources/config/worker.xml Outdated

<service id="worker.command.run" class="Symfony\Component\Worker\Command\WorkerRunCommand">
<tag name="console.command" />
<argument type="service" id="worker.worker_locator" /> <!-- worker locator -->
Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 4, 2017

Choose a reason for hiding this comment

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

is this comment useful?

src/Symfony/Component/Worker/Command/WorkerListCommand.php Outdated
$io = new SymfonyStyle($input, $output);

if (!$this->workers) {
$io->getErrorStyle()->error('There is no available worker.');
Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 4, 2017

Choose a reason for hiding this comment

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

there are no available workers

src/Symfony/Component/Worker/Command/WorkerListCommand.php
return;
}

$io->getErrorStyle()->title('Available workers');
Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 4, 2017

Choose a reason for hiding this comment

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

why did you get the errorstyle?

Copy link
Copy Markdown
Author

@chalasr chalasr Jul 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Titles are not useful to redirect from stdout, this makes use of stderr

Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 5, 2017

Choose a reason for hiding this comment

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

👍

src/Symfony/Component/Worker/Command/WorkerRunCommand.php

protected function execute(InputInterface $input, OutputInterface $output)
{
$workerName = $input->getArgument('worker');
Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 4, 2017

Choose a reason for hiding this comment

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

why did you remove the check on the availability of the pcntl extension ?

Copy link
Copy Markdown
Author

@chalasr chalasr Jul 5, 2017

Choose a reason for hiding this comment

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

because the command has been moved in Worker which requires ext-pcntl, this code can't be reached without pcntl enabled

Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 5, 2017

Choose a reason for hiding this comment

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

If I required symfony/symfony (and not symfony/worker), it does.

Copy link
Copy Markdown
Author

@chalasr chalasr Jul 5, 2017

Choose a reason for hiding this comment

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

Are you sure? If symfony/symfony requires symfony/worker, symfony/symfony should not be installable without filling all symfony/worker's requirements, right?

Copy link
Copy Markdown
Owner

@lyrixx lyrixx Jul 5, 2017

Choose a reason for hiding this comment

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

symfony/symfony does not require symfony/worker.

Copy link
Copy Markdown
Author

@chalasr chalasr Jul 5, 2017

Choose a reason for hiding this comment

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

of course, sorry. re-added

@chalasr chalasr force-pushed the amqp-worker-commands branch 3 times, most recently from 9a03aff to 9dc3384 Compare July 5, 2017 16:54
@chalasr chalasr force-pushed the amqp-worker-commands branch from 9dc3384 to d36c1bf Compare July 5, 2017 16:55
@lyrixx lyrixx merged commit dbc6c11 into lyrixx:amqp-worker Jul 6, 2017
@chalasr chalasr deleted the amqp-worker-commands branch July 6, 2017 07:54
lyrixx pushed a commit that referenced this pull request Mar 11, 2024
@chalasr
bug symfony#54167 [Messenger] [Amqp] Handle AMQPConnectionException w…
e43b198 
…hen publishing a message. (jwage)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony#36538 Fix symfony#48241
| License       | MIT

If you have a message handler that dispatches messages to another queue, you can encounter `AMQPConnectionException` with the message "Library error: a SSL error occurred" or "a socket error occurred"  depending on if you are using tls or not or if you are running behind a load balancer or not.

You can manually reproduce this issue by dispatching a message where the handler then dispatches another message to a different queue, then go to rabbitmq admin and close the connection manually, then dispatch another message and when the message handler goes to dispatch the other message, you will get this exception:

```
a socket error occurred
#0 /vagrant/vendor/symfony/amqp-messenger/Transport/AmqpTransport.php(60): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpSender->send()
#1 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(62): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpTransport->send()
#2 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
#3 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(61): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
#4 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
#5 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
#6 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
#7 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
#8 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
#9 /vagrant/src/Messenger/MessageBus.php(37): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
#10 /vagrant/vendor/symfony/mailer/Mailer.php(66): App\Messenger\MessageBus->dispatch()
#11 /vagrant/src/Mailer/Mailer.php(83): Symfony\Component\Mailer\Mailer->send()
#12 /vagrant/src/Mailer/Mailer.php(96): App\Mailer\Mailer->send()
#13 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(118): App\Mailer\Mailer->sendEmail()
#14 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(72): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->handle()
#15 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(152): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->__invoke()
symfony#16 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(91): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->callHandler()
symfony#17 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(71): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->handle()
symfony#18 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
symfony#19 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(68): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
symfony#20 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
symfony#21 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
symfony#22 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
symfony#23 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
symfony#24 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
symfony#25 /vagrant/vendor/symfony/messenger/RoutableMessageBus.php(54): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
symfony#26 /vagrant/vendor/symfony/messenger/Worker.php(162): Symfony\Component\Messenger\RoutableMessageBus->dispatch()
symfony#27 /vagrant/vendor/symfony/messenger/Worker.php(109): Symfony\Component\Messenger\Worker->handleMessage()
symfony#28 /vagrant/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(238): Symfony\Component\Messenger\Worker->run()
symfony#29 /vagrant/vendor/symfony/console/Command/Command.php(326): Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute()
symfony#30 /vagrant/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
symfony#31 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand()
symfony#32 /vagrant/vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand()
symfony#33 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun()
symfony#34 /vagrant/vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun()
symfony#35 /vagrant/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php(49): Symfony\Component\Console\Application->run()
symfony#36 /vagrant/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run()
symfony#37 /vagrant/bin/console(11): require_once('...')
symfony#38 {main}
```

TODO:

- [x] Add test for retry logic when publishing messages

Commits
-------

f123370 [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.
lyrixx pushed a commit that referenced this pull request Sep 30, 2024
@nicolas-grekas
bug symfony#58332 [Console] Suppress proc_open errors within `Termi…
2be812d 
…nal::readFromProcess` (fritzmg)

This PR was merged into the 5.4 branch.

Discussion
----------

[Console] Suppress `proc_open` errors within `Terminal::readFromProcess`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

When instantiating `SymfonyStyle` in a command it will try to determine the maximum width of the current console interface.

https://github.com/symfony/symfony/blob/6687e4ea35f45ebd73fb0315938103628cfb13a0/src/Symfony/Component/Console/Style/SymfonyStyle.php#L53

This will execute `stty -a | grep columns` down the line. Access to `stty` might be disallowed however, resulting in the following error:

```
ErrorException: Warning: proc_open(): Exec failed: Permission denied
symfony#16 /vendor/symfony/console/Terminal.php(220): Symfony\Component\Console\Terminal::readFromProcess
#15 /vendor/symfony/console/Terminal.php(204): Symfony\Component\Console\Terminal::getSttyColumns
#14 /vendor/symfony/console/Terminal.php(170): Symfony\Component\Console\Terminal::initDimensionsUsingStty
#13 /vendor/symfony/console/Terminal.php(153): Symfony\Component\Console\Terminal::initDimensions
#12 /vendor/symfony/console/Terminal.php(94): Symfony\Component\Console\Terminal::getWidth
#11 /vendor/symfony/console/Style/SymfonyStyle.php(55): Symfony\Component\Console\Style\SymfonyStyle::__construct
#10 /vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(136): Symfony\Component\Messenger\Command\ConsumeMessagesCommand::interact
#9 /vendor/symfony/console/Command/Command.php(311): Symfony\Component\Console\Command\Command::run
```

(Stack Trace actually from Symfony 6)

The phpDoc of `Terminal::getSttyColumns` states

> Runs and parses stty -a if it's available, _suppressing any error output_.

The latter might refer to `['suppress_errors' => true]` (though I am not sure) - which is a Windows only functionality. In any case, since `Terminal::readFromProcess` already checks for

```php
if (!$process = proc_open(…)) {
    return null;
}
```

and

```php
if (!\is_resource($process)) {
    return null;
}
```

upstream in Symfony 6/7, indicating that `proc_open` might fail - this error can additionally be suppressed using `@`. Besides, `Process::start` also uses ``@proc_open`` (added in symfony@099481f "Prevent warning in proc_open()").

Commits
-------

575249a suppress proc_open errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lyrixx lyrixx lyrixx left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chalasr @lyrixx