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

[Messenger] use events consistently in worker #34217

Merged
merged 1 commit into from Nov 5, 2019

Conversation

@Tobion
Copy link
Member

Tobion commented Nov 1, 2019

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.

/**
* @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 = [])

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 1, 2019

Author Member

EventDispatcher is required to make the limit options reliable.

@Tobion Tobion force-pushed the Tobion:messenger-events branch from a25dffc to fe147b6 Nov 1, 2019
@Tobion Tobion added this to the 4.4 milestone Nov 1, 2019
@Tobion Tobion force-pushed the Tobion:messenger-events branch 2 times, most recently from 87db6f8 to ffea7d3 Nov 1, 2019
@Tobion Tobion requested review from weaverryan and removed request for sroze Nov 1, 2019
@Tobion Tobion force-pushed the Tobion:messenger-events branch from ffea7d3 to ffbc91e Nov 2, 2019
private $logger;
private $memoryResolver;
public function __construct(int $memoryLimit, LoggerInterface $logger = null, callable $memoryResolver = null)

This comment has been minimized.

Copy link
@OskarStark

OskarStark Nov 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 2, 2019

Author Member

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

Copy link
Contributor

OskarStark left a comment

Great work 👏

@Tobion Tobion force-pushed the Tobion:messenger-events branch from ffbc91e to 8b767a9 Nov 2, 2019
Copy link
Member

weaverryan left a comment

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

@fabpot
fabpot approved these changes Nov 4, 2019
Copy link
Member

nicolas-grekas left a comment

(with minor comments)

@Tobion Tobion force-pushed the Tobion:messenger-events branch from 8b767a9 to 9911a6d Nov 5, 2019
@nicolas-grekas nicolas-grekas force-pushed the Tobion:messenger-events branch from 9911a6d to 201f159 Nov 5, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 5, 2019

Thank you @Tobion.

nicolas-grekas added a commit that referenced this pull request Nov 5, 2019
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 merged commit 201f159 into symfony:4.4 Nov 5, 2019
0 of 3 checks passed
0 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Doing some proofreading and checking your coding style.
Details
@Tobion Tobion deleted the Tobion:messenger-events branch Nov 5, 2019
wouterj added a commit to symfony/symfony-docs that referenced this pull request Nov 9, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.