-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Added StopWorkerException #39623
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
Conversation
lyrixx
commented
Dec 23, 2020
•
edited
Loading
edited
Q | A |
---|---|
Branch? | 5.4 |
Bug fix? | no |
New feature? | yes |
Deprecations? | no |
Tickets | Fix #39491 |
License | MIT |
Doc PR |
src/Symfony/Component/Messenger/Exception/StopWorkerException.php
Outdated
Show resolved
Hide resolved
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.
I'd rather use an interface to let people add behavior on existing Exception (like we do with
UnrecoverableExceptionInterface)
We already have a mechanism to stop the worker using eventDispatcher.
This can be used at userland to achieve the same behavior.
class StopWorkerOnCustomLogicListener implements EventSubscriberInterface
{
private $run = true;
public function onMessageFailed(WorkerMessageFailedEvent $event): void
{
if ($event->getThrowable() instanceof Whatever) {
$this->run = false;
}
}
public function onWorkerRunning(WorkerRunningEvent $event): void
{
if (!$event->isWorkerIdle() && !$this->run) {
$this->run = true;
$event->getWorker()->stop();
}
}
public static function getSubscribedEvents(): array
{
return [
WorkerMessageFailedEvent::class => 'onMessageFailed',
WorkerRunningEvent::class => 'onWorkerRunning',
];
}
}
I dont follow you, I could create an interface, and implementing it, but what could you add to the Exception? I'll not used anyway
Yes I know, but this is more complex IMHO. Having something native is way more efficient |
hmm.. nothing, It's just a Marker interface interface StopWorkerExceptionInterface extends \Throwable
{
}
class StopWorkerException extends RuntimeException implements StopWorkerExceptionInterface
{
}
if ($throwable instanceof StopWorkerExceptionInterface) {
$this->stop();
} That would allow people use class MyBusinessException implements StopWorkerExceptionInterface
{
} Instead of try {
} catch (MyBusinessException $e) {
throw new StopWorkerExceptionInterface(??, ??, $e);
} |
I see how much this can be handy and simple, but throwing such an exception from e.g. a message handler feels weird to me. Stopping the worker is a consequence of an error, not an error on its own. |
I agree this PR could leverage the Event Dispatcher System. But messenger could work without messenger, so the exception would become useless... But I think we don't care. But IMHO this should not be a doc cookbook. The proposed system is really hackish. Many (good) developers did not find it by them-self. Let's add a native / simple behavior here. |
ae59fa0
to
2b8f6c2
Compare
|
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.
should not we change HandleMessageMiddleware
instead and avoid to add one listener:
up: ok, I saw that it was the previous version 👌
} catch (\Throwable $e) {
if ($e instanceof StopWorkerExceptionInterface) {
throw $e;
}
$exceptions[] = $e;
}
src/Symfony/Component/Messenger/EventListener/StopWorkerOnCustomStopExceptionListener.php
Show resolved
Hide resolved
I have rebased this PR and added some tests. There are 9 👍🏼 on the issue, so I do think we should consider to merge this PR |
Thank you @lyrixx. |