Skip to content
This repository has been archived by the owner on May 25, 2020. It is now read-only.

Error with application state resetting (when http kernel generates an exception) #42

Closed
tarampampam opened this issue May 10, 2020 · 3 comments · Fixed by #43
Closed
Assignees
Labels
priority:high type:bug Something isn't working

Comments

@tarampampam
Copy link
Contributor

Describe the bug

In exceptional situations (eg.: some important external services, that used by application - is unavailable) application state between requests are not reseted.

When some error occured - first request generates error, second shows "random" application page.

Also I think - behaviour can be reproduced then exception will be thrown on HTTP middleware termination.

Expected behaviour

Each incoming request must be processed with "fresh" application state.

System information

Please, complete the following information:

  • PHP version(s): 7.2.14 (in docker)
  • Package version(s): 3.1.0

Additional context

Error screenshot:

image

"Fast fix" idea:

diff --git a/src/Worker.php b/src/Worker.php
index 8f102d1..75c3759 100644
--- a/src/Worker.php
+++ b/src/Worker.php
@@ -71,18 +71,27 @@ class Worker implements WorkerInterface
             $http_kernel = $sandbox->make(HttpKernelContract::class);
 
             try {
-                $this->fireEvent($sandbox, new Events\BeforeLoopIterationEvent($sandbox, $req));
-                $request = Request::createFromBase($http_factory->createRequest($req));
-
-                $this->fireEvent($sandbox, new Events\BeforeRequestHandlingEvent($sandbox, $request));
-                $response = $http_kernel->handle($request);
-                $this->fireEvent($sandbox, new Events\AfterRequestHandlingEvent($sandbox, $request, $response));
-
-                $psr7_response = $psr7_factory->createResponse($response);
-                $psr7_client->respond($psr7_response);
-                $http_kernel->terminate($request, $response);
-
-                $this->fireEvent($sandbox, new Events\AfterLoopIterationEvent($sandbox, $request, $response));
+                try {
+                    $this->fireEvent($sandbox, new Events\BeforeLoopIterationEvent($sandbox, $req));
+                    $request = Request::createFromBase($http_factory->createRequest($req));
+
+                    $this->fireEvent($sandbox, new Events\BeforeRequestHandlingEvent($sandbox, $request));
+                    $response = $http_kernel->handle($request);
+                    $this->fireEvent($sandbox, new Events\AfterRequestHandlingEvent($sandbox, $request, $response));
+
+                    $psr7_response = $psr7_factory->createResponse($response);
+                    $psr7_client->respond($psr7_response);
+                    $http_kernel->terminate($request, $response);
+                } catch (\Throwable $e) {
+                    if (
+                        isset($request, $response) && $request instanceof \Symfony\Component\HttpFoundation\Request
+                        && $response instanceof \Symfony\Component\HttpFoundation\Response
+                    ) {
+                        $this->fireEvent($sandbox, new Events\AfterLoopIterationEvent($sandbox, $request, $response));
+                    }
+
+                    throw $e;
+                }
             } catch (\Throwable $e) {
                 $psr7_client->getWorker()->error((string) $e);
             } finally {
@tarampampam tarampampam added the type:bug Something isn't working label May 10, 2020
@jetexe
Copy link
Collaborator

jetexe commented May 12, 2020

Sorry, but the quick fix did not work. We are continuing the investigation

@tarampampam
Copy link
Contributor Author

tarampampam commented May 12, 2020

So, final worker diff is:

diff --git a/src/Worker.php b/src/Worker.php
index 8f102d1..ffa6cb3 100644
--- a/src/Worker.php
+++ b/src/Worker.php
@@ -4,6 +4,7 @@ declare(strict_types = 1);
 
 namespace AvtoDev\RoadRunnerLaravel;
 
+use Throwable;
 use RuntimeException;
 use Illuminate\Http\Request;
 use InvalidArgumentException;
@@ -58,6 +59,8 @@ class Worker implements WorkerInterface
         $this->fireEvent($app, new Events\BeforeLoopStartedEvent($app));
 
         while ($req = $psr7_client->acceptRequest()) {
+            $responded = false;
+
             if ($refresh_app === true) {
                 $sandbox = $this->createApplication($this->base_path);
                 $this->bootstrapApplication($sandbox, $psr7_client);
@@ -69,6 +72,8 @@ class Worker implements WorkerInterface
 
             /** @var HttpKernelContract $http_kernel */
             $http_kernel = $sandbox->make(HttpKernelContract::class);
+            /** @var ConfigRepository $config */
+            $config = $sandbox->make(ConfigRepository::class);
 
             try {
                 $this->fireEvent($sandbox, new Events\BeforeLoopIterationEvent($sandbox, $req));
@@ -80,11 +85,16 @@ class Worker implements WorkerInterface
 
                 $psr7_response = $psr7_factory->createResponse($response);
                 $psr7_client->respond($psr7_response);
+                $responded = true;
                 $http_kernel->terminate($request, $response);
 
                 $this->fireEvent($sandbox, new Events\AfterLoopIterationEvent($sandbox, $request, $response));
-            } catch (\Throwable $e) {
-                $psr7_client->getWorker()->error((string) $e);
+            } catch (Throwable $e) {
+                if ($responded !== true) {
+                    $psr7_client->getWorker()->error($this->exceptionToString($e, $this->isDebugModeEnabled($config)));
+                }
+
+                $this->fireEvent($sandbox, new Events\LoopErrorOccurredEvent($sandbox, $req, $e));
             } finally {
                 unset($http_kernel, $response, $request, $sandbox);
 
@@ -95,6 +105,29 @@ class Worker implements WorkerInterface
         $this->fireEvent($app, new Events\AfterLoopStoppedEvent($app));
     }
 
+    /**
+     * @param Throwable $e
+     * @param bool      $is_debug
+     *
+     * @return string
+     */
+    protected function exceptionToString(Throwable $e, bool $is_debug): string
+    {
+        return $is_debug
+            ? (string) $e
+            : 'Internal server error';
+    }
+
+    /**
+     * @param ConfigRepository $config
+     *
+     * @return bool
+     */
+    protected function isDebugModeEnabled(ConfigRepository $config): bool
+    {
+        return $config->get('app.debug', false) === true;
+    }
+
     /**
      * @param ApplicationContract $app
      *

already in #43

@tarampampam
Copy link
Contributor Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority:high type:bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants