8000 [Messenger] use events consistently in worker by Tobion · Pull Request #34217 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] use events consistently in worker #34217

New issu 8000 e

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2019
Merged

[Messenger] use events consistently in worker #34217

merged 1 commit into from
Nov 5, 2019

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Nov 1, 2019 •
edited
Loading
Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #32560, #32614, #33843
License MIT
Doc PR

The worker had the three ways to handle events

  1. $onHandledCallback in run(array $options = [], callable $onHandledCallback = null)
  2. events dispatched using the event dispatcher
  3. hardcoded things inside the worker

This PR refactores the messenger worker to only use event dispatching. So instead of a hardcoded $onHandledCallback and worker decorators, we use event listeners and we don't need a WorkerInterface at all. The behavior of all the options like --memory-limit etc remains the same.

I introduced two new events

  • WorkerStartedEvent
  • WorkerRunningEvent

Together with the existing WorkerStoppedEvent it's very symmetrical and solves the referenced issues.

@Tobion Tobion requested a review from sroze as a code owner November 1, 2019 21:41
Tobion
Tobion commented Nov 1, 2019
View reviewed changes
src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

/**
* @param RoutableMessageBus $routableBus
*/
public function __construct($routableBus, ContainerInterface $receiverLocator, LoggerInterface $logger = null, array $receiverNames = [], /* EventDispatcherInterface */ $eventDispatcher = null)
public function __construct($routableBus, ContainerInterface $receiverLocator, EventDispatcherInterface $eventDispatcher, LoggerInterface $logger = null, array $receiverNames = [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EventDispatcher is required to make the limit options reliable.

@Tobion Tobion added this to the 4.4 milestone Nov 1, 2019
@Tobion Tobion force-pushed the messenger-events branch 2 times, most recently from 87db6f8 to ffea7d3 Compare November 1, 2019 22:28
@Tobion Tobion requested review from weaverryan and removed request for sroze November 1, 2019 22:28
@Tobion Tobion mentioned this pull request Nov 1, 2019
Closed
OskarStark
OskarStark reviewed Nov 2, 2019
View reviewed changes
src/Symfony/Component/Messenger/EventListener/StopWorkerOnMemoryLimitListener.php
private $logger;
private $memoryResolver;

public function __construct(int $memoryLimit, LoggerInterface $logger = null, callable $memoryResolver = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it make sense to make the logger the third argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The memory resolver is mainly for testing and it has been like this before.

OskarStark
OskarStark reviewed Nov 2, 2019
View reviewed changes
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Great work 👏

weaverryan
weaverryan reviewed Nov 3, 2019
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is incredible. It's another very nice cleanup with no downside. Big +1 from me after the minor notes.

This was referenced Nov 3, 2019
Merged
Merged
@pkruithof pkruithof mentioned this pull request Nov 5, 2019
Merged
nicolas-grekas
nicolas-grekas approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with minor comments)

@nicolas-grekas
Copy link
Member

Thank you @Tobion.

nicolas-grekas added a commit that referenced this pull request Nov 5, 2019
@nicolas-grekas
feature #34217 [Messenger] use events consistently in worker (Tobion)
a0cefaa 
This PR was merged into the 4.4 branch.

Discussion
----------

[Messenger] use events consistently in worker

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #32560, #32614, #33843
| License       | MIT
| Doc PR        |

The worker had the three ways to handle events
1. $onHandledCallback in `run(array $options = [], callable $onHandledCallback = null)`
2. events dispatched using the event dispatcher
3. hardcoded things inside the worker

This PR refactores the messenger worker to only use event dispatching. So instead of a hardcoded `$onHandledCallback` and worker decorators, we use event listeners and we don't need a `WorkerInterface` at all. The behavior of all the options like `--memory-limit` etc remains the same.

I introduced two new events
- `WorkerStartedEvent`
- `WorkerRunningEvent`

Together with the existing `WorkerStoppedEvent` it's very symmetrical and solves the referenced issues.

Commits
-------

201f159 [Messenger] use events consistently in worker
@nicolas-grekas nicolas-grekas mentioned this pull request Nov 5, 2019
Closed
@nicolas-grekas nicolas-grekas merged commit 201f159 into symfony:4.4 Nov 5, 2019
@Tobion Tobion deleted the messenger-events branch November 5, 2019 21:57
wouterj added a commit to symfony/symfony-docs that referenced this pull request Nov 9, 2019
@wouterj
feature #12586 [Messenger] update events for 4.4 (Tobion)
3ef26b4 
This PR was merged into the 4.4 branch.

Discussion
----------

[Messenger] update events for 4.4

Documentation for symfony/symfony#34217

Commits
-------

3374261 [Messenger] update events for 4.4
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkruithof pkruithof pkruithof left review comments

@OskarStark OskarStark OskarStark left review comments

@chalasr chalasr chalasr left review comments

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature Messenger Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

[Messenger] Custom method argument $onHandledCallback for worker with command messenger:consume
8 participants
@Tobion @nicolas-grekas @fabpot @weaverryan @pkruithof @OskarStark @chalasr @carsonbot
0