Skip to content

[Messenger] make TraceableMiddleware decorate a StackInterface instead of each middleware to free the callstack from noisy frames #29006

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

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.2
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Right now, TraceableMiddleware works as a middleware decorator for the "handle" method. It means that each middleware frame in the call stack is wrapped between two noisy extra frames.
This is something we can remove by decorating the StackInterface instead: let's just stop/start events when the next middleware is fetched.
Thanks to this, only one frame per bus is added to measure middleware timings.

Copy link
Contributor

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

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

Much better, thanks!

private $stopwatch;
private $busName;
private $eventCategory;

public function __construct(MiddlewareInterface $inner, Stopwatch $stopwatch, string $busName = null, string $eventCategory = 'messenger.middleware')
public function __construct(Stopwatch $stopwatch, string $busName = null, string $eventCategory = 'messenger.middleware')
Copy link
Contributor

Choose a reason for hiding this comment

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

- string $busName = null
+ string $busName

? It's mandatory in TraceableStack which makes sense now as we don't have the shared middleware issue anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

works for me, updated, thanks.

…d of each middleware to free the callstack from noisy frames
@nicolas-grekas nicolas-grekas force-pushed the messenger-traceable-stack branch from e97e429 to 73c8c23 Compare October 29, 2018 20:19
@nicolas-grekas nicolas-grekas merged commit 73c8c23 into symfony:master Oct 30, 2018
nicolas-grekas added a commit that referenced this pull request Oct 30, 2018
…nterface instead of each middleware to free the callstack from noisy frames (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] make TraceableMiddleware decorate a StackInterface instead of each middleware to free the callstack from noisy frames

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Right now, `TraceableMiddleware` works as a middleware decorator for the "handle" method. It means that *each middleware* frame in the call stack is wrapped between two noisy extra frames.
This is something we can remove by decorating the StackInterface instead: let's just stop/start events when the next middleware is fetched.
Thanks to this, only one frame *per bus* is added to measure middleware timings.

Commits
-------

73c8c23 [Messenger] make TraceableMiddleware decorate a StackInterface instead of each middleware to free the callstack from noisy frames
@sroze sroze deleted the messenger-traceable-stack branch October 30, 2018 15:19
sroze added a commit that referenced this pull request Oct 31, 2018
…arent interfaces receive *all* matching messages, wildcard included (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

 [Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

~Embeds #29006 for now.~

From the CHANGELOG:
 * senders and handlers subscribing to parent interfaces now receive *all* matching messages, wildcard included
 * `HandlerLocatorInterface::resolve()` has been removed, use `HandlersLocator::getHandlers()` instead
 * `SenderLocatorInterface::getSenderForMessage()` has been removed, use `SendersLocator::getSenders()` instead
 * The `ChainHandler` and `ChainSender` classes have been removed
 * The `ContainerHandlerLocator`, `AbstractHandlerLocator`, `SenderLocator` and `AbstractSenderLocator` classes have been removed

The new `HandlersLocatorInterface` and `SendersLocatorInterface` interfaces return **`iterable`** of corresponding handlers/senders. This allows simplifying a lot the DI configuration and standalone usage.

Inheritance-based configuration is now stable: when a sender or a handler is bound to `SomeMessageInterface`, it will always get all messages of that kind. This fixes the unstable nature of the previous logic, where only the first matching type bound to a message would match, making routing/handling unpredictable (note that the new behavior is also how Laravel does it.)

Some cleanups found meanwhile:
 * the `messenger.sender` tag was useless, it's removed
 * the reponsibility of the "send-and-handle" decision has been moved to the locator, where it belongs
 * thanks to type+argument autowiring aliases, we don't need to force defining a default bus - gaining nice errors when a named argument has a typo
 * some services have been renamed to make them more conventional

As far as priorities are concerned, the priority number applies in the scope of the type bound to it: senders/handlers that are bound to more specific types are always called before less specific ones, no matter the defined priority.

Commits
-------

1e7af4d [Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included
This was referenced Nov 3, 2018
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.

3 participants