Skip to content

Commit e97e429

Browse files
nicolas-grekasnicolas-grekas
committed
[Messenger] make TraceableMiddleware decorate a StackInterface instead of each middleware to free the callstack from noisy frames
1 parent b6758e9 commit e97e429

File tree

6 files changed

+64
-110
lines changed

6 files changed

+64
-110
lines changed

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+5-1
Original file line numberDiff line numberDiff line change
@@ -1542,11 +1542,15 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
15421542
}
15431543

15441544
foreach ($middleware as $middlewareItem) {
1545-
if (!$validationConfig['enabled'] && 'messenger.middleware.validation' === $middlewareItem['id']) {
1545+
if (!$validationConfig['enabled'] && \in_array($middlewareItem['id'], array('validation', 'messenger.middleware.validation'), true)) {
15461546
throw new LogicException('The Validation middleware is only available when the Validator component is installed and enabled. Try running "composer require symfony/validator".');
15471547
}
15481548
}
15491549

1550+
if ($container->getParameter('kernel.debug') && class_exists(Stopwatch::class)) {
1551+
array_unshift($middleware, array('id' => 'traceable', 'arguments' => array($busId)));
1552+
}
1553+
15501554
$container->setParameter($busId.'.middleware', $middleware);
15511555
$container->register($busId, MessageBus::class)->addArgument(array())->addTag('messenger.bus');
15521556

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml

+6-4
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,14 @@
3333
<argument type="service" id="validator" />
3434
</service>
3535

36+
<service id="messenger.middleware.traceable" class="Symfony\Component\Messenger\Middleware\TraceableMiddleware" abstract="true">
37+
<argument type="service" id="debug.stopwatch" />
38+
</service>
39+
3640
<!-- Logging -->
3741
<service id="messenger.middleware.logging" class="Symfony\Component\Messenger\Middleware\LoggingMiddleware" abstract="true">
38-
<argument type="service" id="logger" />
39-
4042
<tag name="monolog.logger" channel="messenger" />
43+
<argument type="service" id="logger" />
4144
</service>
4245

4346
<!-- Discovery -->
@@ -56,10 +59,9 @@
5659
</service>
5760

5861
<service id="messenger.transport.amqp.factory" class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
62+
<tag name="messenger.transport_factory" />
5963
<argument type="service" id="messenger.transport.serializer" />
6064
<argument>%kernel.debug%</argument>
61-
62-
<tag name="messenger.transport_factory" />
6365
</service>
6466
</services>
6567
</container>

‎src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

+4-22
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\Messenger\DependencyInjection;
1313

14+
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
1415
use Symfony\Component\DependencyInjection\ChildDefinition;
1516
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
1617
use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
@@ -22,7 +23,6 @@
2223
use Symfony\Component\Messenger\Handler\ChainHandler;
2324
use Symfony\Component\Messenger\Handler\Locator\ContainerHandlerLocator;
2425
use Symfony\Component\Messenger\Handler\MessageSubscriberInterface;
25-
use Symfony\Component\Messenger\Middleware\TraceableMiddleware;
2626
use Symfony\Component\Messenger\TraceableMessageBus;
2727
use Symfony\Component\Messenger\Transport\Receiver\ReceiverInterface;
2828
use Symfony\Component\Messenger\Transport\Sender\SenderInterface;
@@ -38,15 +38,13 @@ class MessengerPass implements CompilerPassInterface
3838
private $busTag;
3939
private $senderTag;
4040
private $receiverTag;
41-
private $debugStopwatchId;
4241

43-
public function __construct(string $handlerTag = 'messenger.message_handler', string $busTag = 'messenger.bus', string $senderTag = 'messenger.sender', string $receiverTag = 'messenger.receiver', string $debugStopwatchId = 'debug.stopwatch')
42+
public function __construct(string $handlerTag = 'messenger.message_handler', string $busTag = 'messenger.bus', string $senderTag = 'messenger.sender', string $receiverTag = 'messenger.receiver')
4443
{
4544
$this->handlerTag = $handlerTag;
4645
$this->busTag = $busTag;
4746
$this->senderTag = $senderTag;
4847
$this->receiverTag = $receiverTag;
49-
$this->debugStopwatchId = $debugStopwatchId;
5048
}
5149

5250
/**
@@ -306,7 +304,6 @@ private function registerBusToCollector(ContainerBuilder $container, string $bus
306304

307305
private function registerBusMiddleware(ContainerBuilder $container, string $busId, array $middlewareCollection)
308306
{
309-
$debug = $container->getParameter('kernel.debug') && $container->has($this->debugStopwatchId);
310307
$middlewareReferences = array();
311308
foreach ($middlewareCollection as $middlewareItem) {
312309
$id = $middlewareItem['id'];
@@ -319,7 +316,7 @@ private function registerBusMiddleware(ContainerBuilder $container, string $busI
319316
throw new RuntimeException(sprintf('Invalid middleware "%s": define such service to be able to use it.', $id));
320317
}
321318

322-
if ($isDefinitionAbstract = ($definition = $container->findDefinition($messengerMiddlewareId))->isAbstract()) {
319+
if (($definition = $container->findDefinition($messengerMiddlewareId))->isAbstract()) {
323320
$childDefinition = new ChildDefinition($messengerMiddlewareId);
324321
$count = \count($definition->getArguments());
325322
foreach (array_values($arguments ?? array()) as $key => $argument) {
@@ -333,24 +330,9 @@ private function registerBusMiddleware(ContainerBuilder $container, string $busI
333330
throw new RuntimeException(sprintf('Invalid middleware factory "%s": a middleware factory must be an abstract definition.', $id));
334331
}
335332

336-
if ($debug) {
337-
$container->register($debugMiddlewareId = '.messenger.debug.traced.'.$messengerMiddlewareId, TraceableMiddleware::class)
338-
// Decorates with a high priority so it's applied the earliest:
339-
->setDecoratedService($messengerMiddlewareId, null, 100)
340-
->setArguments(array(
341-
new Reference($debugMiddlewareId.'.inner'),
342-
new Reference($this->debugStopwatchId),
343-
// In case the definition isn't abstract,
344-
// we cannot be sure the service instance is used by one bus only.
345-
// So we only inject the bus name when the original definition is abstract.
346-
$isDefinitionAbstract ? $busId : null,
347-
))
348-
;
349-
}
350-
351333
$middlewareReferences[] = new Reference($messengerMiddlewareId);
352334
}
353335

354-
$container->getDefinition($busId)->replaceArgument(0, $middlewareReferences);
336+
$container->getDefinition($busId)->replaceArgument(0, new IteratorArgument($middlewareReferences));
355337
}
356338
}

‎src/Symfony/Component/Messenger/Middleware/TraceableMiddleware.php

+30-31
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,12 @@
2121
*/
2222
class TraceableMiddleware implements MiddlewareInterface
2323
{
24-
private $inner;
2524
private $stopwatch;
2625
private $busName;
2726
private $eventCategory;
2827

29-
public function __construct(MiddlewareInterface $inner, Stopwatch $stopwatch, string $busName = null, string $eventCategory = 'messenger.middleware')
28+
public function __construct(Stopwatch $stopwatch, string $busName = null, string $eventCategory = 'messenger.middleware')
3029
{
31-
$this->inner = $inner;
3230
$this->stopwatch = $stopwatch;
3331
$this->busName = $busName;
3432
$this->eventCategory = $eventCategory;
@@ -39,64 +37,65 @@ public function __construct(MiddlewareInterface $inner, Stopwatch $stopwatch, st
3937
*/
4038
public function handle(Envelope $envelope, StackInterface $stack): Envelope
4139
{
42-
$class = \get_class($this->inner);
43-
$eventName = 'c' === $class[0] && 0 === strpos($class, "class@anonymous\0") ? get_parent_class($class).'@anonymous' : $class;
44-
45-
if ($this->busName) {
46-
$eventName .= " (bus: {$this->busName})";
47-
}
48-
49-
$this->stopwatch->start($eventName, $this->eventCategory);
40+
$stack = new TraceableStack($stack, $this->stopwatch, $this->busName, $this->eventCategory);
5041

5142
try {
52-
return $this->inner->handle($envelope, new TraceableInnerMiddleware($stack, $this->stopwatch, $eventName, $this->eventCategory));
43+
return $stack->next()->handle($envelope, $stack);
5344
} finally {
54-
if ($this->stopwatch->isStarted($eventName)) {
55-
$this->stopwatch->stop($eventName);
56-
}
45+
$stack->stop();
5746
}
5847
}
5948
}
6049

6150
/**
6251
* @internal
6352
*/
64-
class TraceableInnerMiddleware implements MiddlewareInterface, StackInterface
53+
class TraceableStack implements StackInterface
6554
{
6655
private $stack;
6756
private $stopwatch;
68-
private $eventName;
57+
private $busName;
6958
private $eventCategory;
59+
private $currentEvent;
7060

71-
public function __construct(StackInterface $stack, Stopwatch $stopwatch, string $eventName, string $eventCategory)
61+
public function __construct(StackInterface $stack, Stopwatch $stopwatch, string $busName, string $eventCategory)
7262
{
7363
$this->stack = $stack;
7464
$this->stopwatch = $stopwatch;
75-
$this->eventName = $eventName;
65+
$this->busName = $busName;
7666
$this->eventCategory = $eventCategory;
7767
}
7868

7969
/**
8070
* {@inheritdoc}
8171
*/
82-
public function handle(Envelope $envelope, StackInterface $stack): Envelope
72+
public function next(): MiddlewareInterface
8373
{
84-
$this->stopwatch->stop($this->eventName);
85-
if ($this === $stack) {
86-
$envelope = $this->stack->next()->handle($envelope, $this->stack);
74+
if (null !== $this->currentEvent) {
75+
$this->stopwatch->stop($this->currentEvent);
76+
}
77+
78+
if ($this->stack === $nextMiddleware = $this->stack->next()) {
79+
$this->currentEvent = 'Tail';
8780
} else {
88-
$envelope = $stack->next()->handle($envelope, $stack);
81+
$class = \get_class($nextMiddleware);
82+
$this->currentEvent = 'c' === $class[0] && 0 === strpos($class, "class@anonymous\0") ? get_parent_class($class).'@anonymous' : $class;
8983
}
90-
$this->stopwatch->start($this->eventName, $this->eventCategory);
9184

92-
return $envelope;
85+
if ($this->busName) {
86+
$this->currentEvent .= " (bus: {$this->busName})";
87+
}
88+
89+
$this->stopwatch->start($this->currentEvent, $this->eventCategory);
90+
91+
return $nextMiddleware;
9392
}
9493

95-
/**
96-
* {@inheritdoc}
97-
*/
98-
public function next(): MiddlewareInterface
94+
public function stop()
9995
{
100-
return $this;
96+
if (null !== $this->currentEvent && $this->stopwatch->isStarted($this->currentEvent)) {
97+
$this->stopwatch->stop($this->currentEvent);
98+
}
99+
$this->currentEvent = null;
101100
}
102101
}

‎src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

+1-35
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
use Symfony\Component\Messenger\Transport\AmqpExt\AmqpReceiver;
4242
use Symfony\Component\Messenger\Transport\AmqpExt\AmqpSender;
4343
use Symfony\Component\Messenger\Transport\Receiver\ReceiverInterface;
44-
use Symfony\Component\Stopwatch\Stopwatch;
4544

4645
class MessengerPassTest extends TestCase
4746
{
@@ -524,7 +523,7 @@ public function testRegistersMiddlewareFromServices()
524523
new Reference(UselessMiddleware::class),
525524
new Reference($factoryChildMiddlewareId),
526525
new Reference($factoryWithDefaultChildMiddlewareId),
527-
), $container->getDefinition($fooBusId)->getArgument(0));
526+
), $container->getDefinition($fooBusId)->getArgument(0)->getValues());
528527
$this->assertFalse($container->hasParameter($middlewareParameter));
529528
}
530529

@@ -557,39 +556,6 @@ public function testMiddlewareFactoryDefinitionMustBeAbstract()
557556
(new MessengerPass())->process($container);
558557
}
559558

560-
public function testDecoratesWithTraceableMiddlewareOnDebug()
561-
{
562-
$container = $this->getContainerBuilder();
563-
564-
$container->register($busId = 'message_bus', MessageBusInterface::class)->setArgument(0, array())->addTag('messenger.bus');
565-
$container->register('abstract_middleware', UselessMiddleware::class)->setAbstract(true);
566-
$container->register('concrete_middleware', UselessMiddleware::class);
567-
568-
$container->setParameter($middlewareParameter = $busId.'.middleware', array(
569-
array('id' => 'abstract_middleware'),
570-
array('id' => 'concrete_middleware'),
571-
));
572-
573-
$container->setParameter('kernel.debug', true);
574-
$container->register('debug.stopwatch', Stopwatch::class);
575-
576-
(new MessengerPass())->process($container);
577-
578-
$this->assertNotNull($concreteDef = $container->getDefinition('.messenger.debug.traced.concrete_middleware'));
579-
$this->assertEquals(array(
580-
new Reference('.messenger.debug.traced.concrete_middleware.inner'),
581-
new Reference('debug.stopwatch'),
582-
null,
583-
), $concreteDef->getArguments());
584-
585-
$this->assertNotNull($abstractDef = $container->getDefinition(".messenger.debug.traced.$busId.middleware.abstract_middleware"));
586-
$this->assertEquals(array(
587-
new Reference(".messenger.debug.traced.$busId.middleware.abstract_middleware.inner"),
588-
new Reference('debug.stopwatch'),
589-
$busId,
590-
), $abstractDef->getArguments());
591-
}
592-
593559
public function testItRegistersTheDebugCommand()
594560
{
595561
$container = $this->getContainerBuilder($commandBusId = 'command_bus');

‎src/Symfony/Component/Messenger/Tests/Middleware/TraceableMiddlewareTest.php

+18-17
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Symfony\Component\Messenger\Envelope;
1515
use Symfony\Component\Messenger\Middleware\MiddlewareInterface;
1616
use Symfony\Component\Messenger\Middleware\StackInterface;
17+
use Symfony\Component\Messenger\Middleware\StackMiddleware;
1718
use Symfony\Component\Messenger\Middleware\TraceableMiddleware;
1819
use Symfony\Component\Messenger\Test\Middleware\MiddlewareTestCase;
1920
use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
@@ -27,7 +28,7 @@ class TraceableMiddlewareTest extends MiddlewareTestCase
2728
public function testHandle()
2829
{
2930
$busId = 'command_bus';
30-
$envelope = new Envelope($message = new DummyMessage('Hello'));
31+
$envelope = new Envelope(new DummyMessage('Hello'));
3132

3233
$middleware = $this->getMockBuilder(MiddlewareInterface::class)->getMock();
3334
$middleware->expects($this->once())
@@ -42,16 +43,22 @@ public function testHandle()
4243
$stopwatch->expects($this->once())->method('isStarted')->willReturn(true);
4344
$stopwatch->expects($this->exactly(2))
4445
->method('start')
45-
->with($this->matches('%sMiddlewareInterface%s (bus: command_bus)'), 'messenger.middleware')
46+
->withConsecutive(
47+
array($this->matches('%sMiddlewareInterface%s (bus: command_bus)'), 'messenger.middleware'),
48+
array('Tail (bus: command_bus)', 'messenger.middleware')
49+
)
4650
;
4751
$stopwatch->expects($this->exactly(2))
4852
->method('stop')
49-
->with($this->matches('%sMiddlewareInterface%s (bus: command_bus)'))
53+
->withConsecutive(
54+
array($this->matches('%sMiddlewareInterface%s (bus: command_bus)')),
55+
array('Tail (bus: command_bus)')
56+
)
5057
;
5158

52-
$traced = new TraceableMiddleware($middleware, $stopwatch, $busId);
59+
$traced = new TraceableMiddleware($stopwatch, $busId);
5360

54-
$traced->handle($envelope, $this->getStackMock());
61+
$traced->handle($envelope, new StackMiddleware(new \ArrayIterator(array(null, $middleware))));
5562
}
5663

5764
/**
@@ -61,32 +68,26 @@ public function testHandle()
6168
public function testHandleWithException()
6269
{
6370
$busId = 'command_bus';
64-
$envelope = new Envelope($message = new DummyMessage('Hello'));
6571

6672
$middleware = $this->getMockBuilder(MiddlewareInterface::class)->getMock();
6773
$middleware->expects($this->once())
6874
->method('handle')
69-
->with($envelope, $this->anything())
70-
->will($this->returnCallback(function ($envelope, StackInterface $stack) {
71-
return $stack->next()->handle($envelope, $stack);
72-
}))
75+
->willThrowException(new \RuntimeException('Thrown from next middleware.'))
7376
;
7477

75-
$stack = $this->getThrowingStackMock();
76-
7778
$stopwatch = $this->createMock(Stopwatch::class);
7879
$stopwatch->expects($this->once())->method('isStarted')->willReturn(true);
79-
// Start is only expected to be called once, as an exception is thrown by the next callable:
80-
$stopwatch->expects($this->exactly(1))
80+
// Start/stop are expected to be called once, as an exception is thrown by the next callable
81+
$stopwatch->expects($this->once())
8182
->method('start')
8283
->with($this->matches('%sMiddlewareInterface%s (bus: command_bus)'), 'messenger.middleware')
8384
;
84-
$stopwatch->expects($this->exactly(2))
85+
$stopwatch->expects($this->once())
8586
->method('stop')
8687
->with($this->matches('%sMiddlewareInterface%s (bus: command_bus)'))
8788
;
8889

89-
$traced = new TraceableMiddleware($middleware, $stopwatch, $busId);
90-
$traced->handle($envelope, $stack);
90+
$traced = new TraceableMiddleware($stopwatch, $busId);
91+
$traced->handle(new Envelope(new DummyMessage('Hello')), new StackMiddleware(new \ArrayIterator(array(null, $middleware))));
9192
}
9293
}

0 commit comments

Comments
 (0)