Skip to content

[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

Merged
merged 1 commit into from
Jul 3, 2021
Merged

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Dec 23, 2020

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #39491
License MIT
Doc PR

Copy link
Member

@jderusse jderusse left a 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',
        ];
    }
}

@lyrixx
Copy link
Member Author

lyrixx commented Dec 23, 2020

I'd rather use an interface to let people add behavior on existing Exception (like we do with
UnrecoverableExceptionInterface)

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

We already have a mechanism to stop the worker using eventDispatcher.

Yes I know, but this is more complex IMHO. Having something native is way more efficient
And BTW, 2 people already asked me how do to that :) And the issue receive a good feedback.

@jderusse
Copy link
Member

but what could you add to the Exception?

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);
}

@chalasr
Copy link
Member

chalasr commented Dec 23, 2020

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 that hooking into worker lifecycle events sounds better from a design pov.
Adding a sample listener to the docs like the one showed by @jderusse may be enough to fix this issue and could avoid having several paths for the same goal.

@lyrixx
Copy link
Member Author

lyrixx commented Dec 28, 2020

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.

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Dec 28, 2020
@lyrixx lyrixx force-pushed the worker-stop branch 2 times, most recently from ae59fa0 to 2b8f6c2 Compare December 31, 2020 15:47
@lyrixx
Copy link
Member Author

lyrixx commented Dec 31, 2020

  • I added an interface
  • the system uses the Event Dispatch

Copy link
Contributor

@ottaviano ottaviano left a 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;
}

@lyrixx
Copy link
Member Author

lyrixx commented May 10, 2021

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

@fabpot
Copy link
Member

fabpot commented Jul 3, 2021

Thank you @lyrixx.

@fabpot fabpot merged commit b28b63c into symfony:5.4 Jul 3, 2021
@lyrixx lyrixx deleted the worker-stop branch July 5, 2021 08:26
This was referenced Nov 5, 2021
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.

[RFC][Messenger] allow to shutdown worker "consume" with a particular Exception
8 participants