Skip to content

Commit 0874ea8

Browse files
fabpotfabpot
committed
bug #49706 Stop stopwatch events in case of exception (MatTheCat)
This PR was squashed before being merged into the 5.4 branch. Discussion ---------- Stop stopwatch events in case of exception | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #49677 | License | MIT | Doc PR | N/A Stopwatch events need to be stopped even if an exception occurred, else they will appear to span across the whole request timeline. The following screenshots were taken with `RouterListener` throwing in debug mode following a [`NoConfigurationException`](https://github.com/symfony/symfony/blob/0362350a720e38df55531ab0cf726a082d3d34d4/src/Symfony/Component/Routing/Exception/NoConfigurationException.php): <details> <summary>Before</summary> <img src="https://user-images.githubusercontent.com/243674/224777379-1209dfde-8086-49e3-af56-147ceda3e6c9.png" alt=""> </details> <details> <summary>After</summary> <img src="https://user-images.githubusercontent.com/1898254/225371948-a21acf87-203c-49f9-b73a-4947e9a5092a.png" alt=""> </details> Commits ------- beca17a Stop stopwatch events in case of exception
2 parents 0362350 + beca17a commit 0874ea8

File tree

6 files changed

+125
-14
lines changed

6 files changed

+125
-14
lines changed

‎src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,12 @@ public function __invoke(object $event, string $eventName, EventDispatcherInterf
114114

115115
$e = $this->stopwatch->start($this->name, 'event_listener');
116116

117-
($this->optimizedListener ?? $this->listener)($event, $eventName, $dispatcher);
118-
119-
if ($e->isStarted()) {
120-
$e->stop();
117+
try {
118+
($this->optimizedListener ?? $this->listener)($event, $eventName, $dispatcher);
119+
} finally {
120+
if ($e->isStarted()) {
121+
$e->stop();
122+
}
121123
}
122124

123125
if ($event instanceof StoppableEventInterface && $event->isPropagationStopped()) {

‎src/Symfony/Component/EventDispatcher/Tests/Debug/WrappedListenerTest.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\EventDispatcher\Debug\WrappedListener;
1616
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
1717
use Symfony\Component\Stopwatch\Stopwatch;
18+
use Symfony\Component\Stopwatch\StopwatchEvent;
1819

1920
class WrappedListenerTest extends TestCase
2021
{
@@ -42,6 +43,25 @@ public static function provideListenersToDescribe()
4243
[\Closure::fromCallable(function () {}), 'closure'],
4344
];
4445
}
46+
47+
public function testStopwatchEventIsStoppedWhenListenerThrows()
48+
{
49+
$stopwatchEvent = $this->createMock(StopwatchEvent::class);
50+
$stopwatchEvent->expects(self::once())->method('isStarted')->willReturn(true);
51+
$stopwatchEvent->expects(self::once())->method('stop');
52+
53+
$stopwatch = $this->createStub(Stopwatch::class);
54+
$stopwatch->method('start')->willReturn($stopwatchEvent);
55+
56+
$dispatcher = $this->createStub(EventDispatcherInterface::class);
57+
58+
$wrappedListener = new WrappedListener(function () { throw new \Exception(); }, null, $stopwatch, $dispatcher);
59+
60+
try {
61+
$wrappedListener(new \stdClass(), 'foo', $dispatcher);
62+
} catch (\Exception $ex) {
63+
}
64+
}
4565
}
4666

4767
class FooListener

‎src/Symfony/Component/HttpKernel/Controller/TraceableArgumentResolver.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ public function getArguments(Request $request, callable $controller)
3535
{
3636
$e = $this->stopwatch->start('controller.get_arguments');
3737

38-
$ret = $this->resolver->getArguments($request, $controller);
39-
40-
$e->stop();
41-
42-
return $ret;
38+
try {
39+
return $this->resolver->getArguments($request, $controller);
40+
} finally {
41+
$e->stop();
42+
}
4343
}
4444
}

‎src/Symfony/Component/HttpKernel/Controller/TraceableControllerResolver.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ public function getController(Request $request)
3535
{
3636
$e = $this->stopwatch->start('controller.get_callable');
3737

38-
$ret = $this->resolver->getController($request);
39-
40-
$e->stop();
41-
42-
return $ret;
38+
try {
39+
return $this->resolver->getController($request);
40+
} finally {
41+
$e->stop();
42+
}
4343
}
4444
}

‎src/Symfony/Component/HttpKernel/Tests/Controller/TraceableArgumentResolverTest.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Controller;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface;
17+
use Symfony\Component\HttpKernel\Controller\TraceableArgumentResolver;
18+
use Symfony\Component\Stopwatch\Stopwatch;
19+
use Symfony\Component\Stopwatch\StopwatchEvent;
20+
21+
class TraceableArgumentResolverTest extends TestCase
22+
{
23+
public function testStopwatchEventIsStoppedWhenResolverThrows()
24+
{
25+
$stopwatchEvent = $this->createMock(StopwatchEvent::class);
26+
$stopwatchEvent->expects(self::once())->method('stop');
27+
28+
$stopwatch = $this->createStub(Stopwatch::class);
29+
$stopwatch->method('start')->willReturn($stopwatchEvent);
30+
31+
$resolver = new class() implements ArgumentResolverInterface {
32+
public function getArguments(Request $request, callable $controller)
33+
{
34+
throw new \Exception();
35+
}
36+
};
37+
38+
$traceableResolver = new TraceableArgumentResolver($resolver, $stopwatch);
39+
40+
try {
41+
$traceableResolver->getArguments(new Request(), function () {});
42+
} catch (\Exception $ex) {
43+
}
44+
}
45+
}

‎src/Symfony/Component/HttpKernel/Tests/Controller/TraceableControllerResolverTest.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Controller;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\HttpKernel\Controller\ControllerResolverInterface;
17+
use Symfony\Component\HttpKernel\Controller\TraceableControllerResolver;
18+
use Symfony\Component\Stopwatch\Stopwatch;
19+
use Symfony\Component\Stopwatch\StopwatchEvent;
20+
21+
class TraceableControllerResolverTest extends TestCase
22+
{
23+
public function testStopwatchEventIsStoppedWhenResolverThrows()
24+
{
25+
$stopwatchEvent = $this->createMock(StopwatchEvent::class);
26+
$stopwatchEvent->expects(self::once())->method('stop');
27+
28+
$stopwatch = $this->createStub(Stopwatch::class);
29+
$stopwatch->method('start')->willReturn($stopwatchEvent);
30+
31+
$resolver = new class() implements ControllerResolverInterface {
32+
public function getController(Request $request)
33+
{
34+
throw new \Exception();
35+
}
36+
};
37+
38+
$traceableResolver = new TraceableControllerResolver($resolver, $stopwatch);
39+
try {
40+
$traceableResolver->getController(new Request());
41+
} catch (\Exception $ex) {
42+
}
43+
}
44+
}

0 commit comments

Comments
 (0)