Merge pull request #431 from acelaya/feature/close-db-on-error

Feature/close db on error

module/Common/src/Middleware/CloseDbConnectionMiddleware.php

@@ -3,11 +3,13 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Common\Middleware;
 
+use Doctrine\ORM\EntityManager;
 use Doctrine\ORM\EntityManagerInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
+use Throwable;
 
 class CloseDbConnectionMiddleware implements MiddlewareInterface
 {
@@ -19,16 +21,23 @@ class CloseDbConnectionMiddleware implements MiddlewareInterface
         $this->em = $em;
     }
 
-    /**
-     * Process an incoming server request and return a response, optionally delegating
-     * response creation to a handler.
-     */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
-        $handledRequest =  $handler->handle($request);
-        $this->em->getConnection()->close();
-        $this->em->clear();
+        try {
+            return $handler->handle($request);
+        } catch (Throwable $e) {
+            // FIXME Mega ugly hack to avoid a closed EntityManager to make shlink fail forever on swoole contexts
+            //       Should be fixed with request-shared EntityManagers, which is not supported by the ServiceManager
+            if (! $this->em->isOpen()) {
+                (function () {
+                    $this->closed = false;
+                })->bindTo($this->em, EntityManager::class)();
+            }
 
-        return $handledRequest;
+            throw $e;
+        } finally {
+            $this->em->getConnection()->close();
+            $this->em->clear();
+        }
     }
 }

module/Common/test/Middleware/CloseDbConnectionMiddlewareTest.php

@@ -8,7 +8,9 @@ use Doctrine\ORM\EntityManagerInterface;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Prophecy\ObjectProphecy;
 use Psr\Http\Server\RequestHandlerInterface;
+use RuntimeException;
 use Shlinkio\Shlink\Common\Middleware\CloseDbConnectionMiddleware;
+use Throwable;
 use Zend\Diactoros\Response;
 use Zend\Diactoros\ServerRequest;
 
@@ -20,35 +22,80 @@ class CloseDbConnectionMiddlewareTest extends TestCase
     private $handler;
     /** @var ObjectProphecy */
     private $em;
+    /** @var ObjectProphecy */
+    private $conn;
 
     public function setUp(): void
     {
         $this->handler = $this->prophesize(RequestHandlerInterface::class);
         $this->em = $this->prophesize(EntityManagerInterface::class);
+        $this->conn = $this->prophesize(Connection::class);
+        $this->conn->close()->will(function () {
+        });
+        $this->em->getConnection()->willReturn($this->conn->reveal());
+        $this->em->clear()->will(function () {
+        });
+        $this->em->isOpen()->willReturn(true);
 
         $this->middleware = new CloseDbConnectionMiddleware($this->em->reveal());
     }
 
     /** @test */
-    public function connectionIsClosedWhenMiddlewareIsProcessed()
+    public function connectionIsClosedWhenMiddlewareIsProcessed(): void
     {
         $req = new ServerRequest();
         $resp = new Response();
-
-        $conn = $this->prophesize(Connection::class);
-        $closeConn = $conn->close()->will(function () {
-        });
-        $getConn = $this->em->getConnection()->willReturn($conn->reveal());
-        $clear = $this->em->clear()->will(function () {
-        });
         $handle = $this->handler->handle($req)->willReturn($resp);
 
         $result = $this->middleware->process($req, $this->handler->reveal());
 
         $this->assertSame($result, $resp);
-        $getConn->shouldHaveBeenCalledOnce();
-        $closeConn->shouldHaveBeenCalledOnce();
-        $clear->shouldHaveBeenCalledOnce();
+        $this->em->getConnection()->shouldHaveBeenCalledOnce();
+        $this->conn->close()->shouldHaveBeenCalledOnce();
+        $this->em->clear()->shouldHaveBeenCalledOnce();
         $handle->shouldHaveBeenCalledOnce();
     }
+
+    /** @test */
+    public function connectionIsClosedEvenIfExceptionIsThrownOnInnerMiddlewares(): void
+    {
+        $req = new ServerRequest();
+        $expectedError = new RuntimeException();
+        $this->handler->handle($req)->willThrow($expectedError)
+                                    ->shouldBeCalledOnce();
+
+        $this->em->getConnection()->shouldBeCalledOnce();
+        $this->conn->close()->shouldBeCalledOnce();
+        $this->em->clear()->shouldBeCalledOnce();
+        $this->expectExceptionObject($expectedError);
+
+        $this->middleware->process($req, $this->handler->reveal());
+    }
+
+    /**
+     * @test
+     * @dataProvider provideClosed
+     */
+    public function entityManagerIsReopenedAfterAnExceptionWhichClosedIt(bool $closed): void
+    {
+        $req = new ServerRequest();
+        $expectedError = new RuntimeException();
+        $this->handler->handle($req)->willThrow($expectedError)
+                                    ->shouldBeCalledOnce();
+        $this->em->closed = $closed;
+        $this->em->isOpen()->willReturn(false);
+
+        try {
+            $this->middleware->process($req, $this->handler->reveal());
+            $this->fail('Expected exception to be thrown but it did not.');
+        } catch (Throwable $e) {
+            $this->assertSame($expectedError, $e);
+            $this->assertFalse($this->em->closed);
+        }
+    }
+
+    public function provideClosed(): iterable
+    {
+        return [[true, false]];
+    }
 }

phpstan.neon

@@ -3,3 +3,6 @@ parameters:
 		- '#League\\Plates\\callback#'
 		- '#is not subtype of Throwable#'
 		- '#ObjectManager::flush()#'
+		-
+			message: '#Access to an undefined property#'
+			path: %currentWorkingDirectory%/module/Common/src/Middleware/CloseDbConnectionMiddleware.php