Skip to content
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

[HttpFoundation] session not reset when booting multiple kernels within same php process #30619

Closed
dmaicher opened this issue Mar 20, 2019 · 6 comments

Comments

@dmaicher
Copy link
Contributor

dmaicher commented Mar 20, 2019

Symfony version(s) affected: 3.4 until master

Description
In one of my apps I process messages from AMQP asynchronously using a Symfony command worker (not using symfony/messenger).
Every message is handled in a fresh newly booted kernel to ensure proper isolation.

This worked fine until recently through some changes in our app the session service is used (instantiated & started) during the processing of a message.

Whenever within the same php process now another message gets handled (with a new kernel) we get the following error:

"Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time"

This is because the NativeFileSessionHandler tries to set some ini settings again:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php#L52

And the session from the previous kernel was never closed.

Possible Solution
I discussed this briefly with @nicolas-grekas and to fix this I added the kernel.reset tag to the session service so it can be properly reset. This fixes the issue in my app if I call ServicesResetter::reset() on every kernel before its shut down.

@fabpot fabpot closed this as completed Mar 23, 2019
fabpot added a commit that referenced this issue Mar 23, 2019
…ettable (dmaicher)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle][HttpFoundation] make session service resettable

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

This fixes #30619 by making the session service "resettable"  via `ServicesResetter`.

Commits
-------

e46ef76 [FrameworkBundle][HttpFoundation] make session service resettable
@nicolas-grekas
Copy link
Member

Reopening: #30620 has now been reverted, see #31338 and linked issues/PRs for discussion + possible alternative solution. PR welcome.

@nicolas-grekas nicolas-grekas reopened this May 1, 2019
@dmaicher
Copy link
Contributor Author

@nicolas-grekas I can implement something for 4.4 👍

Is this what you had in mind?

diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
index 020e29e721..198dc35116 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
+++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
@@ -15,6 +15,7 @@
             <argument type="service" id="session.storage" />
             <argument type="service" id="session.attribute_bag" />
             <argument type="service" id="session.flash_bag" />
+            <tag name="kernel.reset" method="reset" />
         </service>
 
         <service id="Symfony\Component\HttpFoundation\Session\SessionInterface" alias="session" />
diff --git a/src/Symfony/Component/HttpFoundation/Session/Session.php b/src/Symfony/Component/HttpFoundation/Session/Session.php
index 0c7e9cc5ca..7f5369da87 100644
--- a/src/Symfony/Component/HttpFoundation/Session/Session.php
+++ b/src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -17,12 +17,13 @@ use Symfony\Component\HttpFoundation\Session\Flash\FlashBag;
 use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
 use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
 use Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface;
+use Symfony\Contracts\Service\ResetInterface;
 
 /**
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Drak <drak@zikula.org>
  */
-class Session implements SessionInterface, \IteratorAggregate, \Countable
+class Session implements SessionInterface, \IteratorAggregate, \Countable, ResetInterface
 {
     protected $storage;
 
@@ -261,6 +262,13 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
         return $this->getBag($this->flashName);
     }
 
+    public function reset()
+    {
+        if ($this->isStarted()) {
+            $this->save();
+        }
+    }
+
     /**
      * Gets the attributebag interface.
      *

The only thing I'm wondering now: Is it really expected that the session is saved when I "reset" it? To me semantically more sense would be to ignore all unsaved changes and reset the service to its initial state (what the contract ResetInterface actually suggests?). WDYT?

@nicolas-grekas
Copy link
Member

Hum, if I had that in mind, that's wrong: the session must be saved just after the response is sent back. Reset is called just before handling the next request.
So, reset must empty the session if it's not already, and a listener ensures save is called already, isn't it?

@dmaicher
Copy link
Contributor Author

dmaicher commented Aug 21, 2019

Yeah you are right 😊

I think this should be enough?

diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
index 020e29e721..c667b95ddc 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
+++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
@@ -15,6 +15,7 @@
             <argument type="service" id="session.storage" />
             <argument type="service" id="session.attribute_bag" />
             <argument type="service" id="session.flash_bag" />
+            <tag name="kernel.reset" method="reset" />
         </service>
 
         <service id="Symfony\Component\HttpFoundation\Session\SessionInterface" alias="session" />
diff --git a/src/Symfony/Component/HttpFoundation/Session/Session.php b/src/Symfony/Component/HttpFoundation/Session/Session.php
index 0c7e9cc5ca..8a3e0adfea 100644
--- a/src/Symfony/Component/HttpFoundation/Session/Session.php
+++ b/src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -17,12 +17,13 @@ use Symfony\Component\HttpFoundation\Session\Flash\FlashBag;
 use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
 use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
 use Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface;
+use Symfony\Contracts\Service\ResetInterface;
 
 /**
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Drak <drak@zikula.org>
  */
-class Session implements SessionInterface, \IteratorAggregate, \Countable
+class Session implements SessionInterface, \IteratorAggregate, \Countable, ResetInterface
 {
     protected $storage;
 
@@ -261,6 +262,11 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
         return $this->getBag($this->flashName);
     }
 
+    public function reset()
+    {
+        $this->clear();
+    }
+
     /**
      * Gets the attributebag interface.
      *

I tested it with my Symfony 4.3 app with this change in the front controller:

$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$kernel->terminate($request, $response);

// handle the same request again
$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);

But now I get a new session id on every page load. WDYT?

@dmaicher
Copy link
Contributor Author

dmaicher commented Aug 21, 2019

@nicolas-grekas to better illustrate my issue that I reported here in the first place:

<?php

require_once 'vendor/autoload.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();
$kernel->getContainer()->get('session')->start();
$kernel->shutdown();

$kernel = new AppKernel('dev', true);
$kernel->boot();
$kernel->getContainer()->get('session')->start();
$kernel->shutdown();

leads to

PHP Fatal error:  Uncaught ErrorException: Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /var/www/project/symfony/vendor/symfony/http-foundation/Session/Storage/Handler/NativeFileSessionHandler.php:52

A possible fix could be:

    public function reset()
    {
        $this->clear();
        if ($this->isStarted()) {
            $this->storage->save();
        }
    }

and

<?php

require_once 'vendor/autoload.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();
$kernel->getContainer()->get('session')->start();
$kernel->getContainer()->get('services_resetter')->reset();
$kernel->shutdown();

$kernel = new AppKernel('dev', true);
$kernel->boot();
$kernel->getContainer()->get('session')->start();
$kernel->shutdown();

Only

public function reset()
{
    $this->clear();
}

is not enough.

Just as mentioned above not sure of the implications if someone re-uses a kernel in http context for multiple different requests. I cannot really test it 😋

@dmaicher
Copy link
Contributor Author

dmaicher commented Mar 2, 2020

Since this seems to be an edge case I'm closing the issue for now

@dmaicher dmaicher closed this as completed Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants