Do not persist session manager

1. +++ b/core/authorize.php
   @@ -47,7 +47,8 @@
   -  \Drupal::service('session_manager')->initialize();
   +  // Initialize the session manager.
   +  \Drupal::service('session_manager');
   
   +++ b/core/includes/install.core.inc
   @@ -1456,7 +1456,8 @@ function install_load_profile(&$install_state) {
   -  \Drupal::service('session_manager')->initialize();
   +  // Initialize the session manager.
   +  \Drupal::service('session_manager');
   
   +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -466,6 +472,9 @@ protected function initializeContainer() {
   +    if ($has_session_manager) {
   +      $this->container->get('session_manager');
   +    }
   
   +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
   @@ -1107,7 +1112,9 @@ protected function rebuildContainer($environment = 'prod') {
   +    if ($has_session_manager) {
   +      $this->container->get('session_manager');
   +    }
   
   +++ b/core/update.php
   @@ -342,7 +342,8 @@ function update_task_list($active = NULL) {
   -\Drupal::service('session_manager')->initialize();
   +// Initialize the session manager.
   +\Drupal::service('session_manager');
   

   All of these calls look like "magic" now — instead of calling initialize() manually (giving sense to the calling code), you rely on the service container to call it for you.

   I could get more behind that if the constructor would internally call initialize(), but that's not the case.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -409,6 +410,11 @@ protected function initializeContainer() {
   +      // If there is a session manager, close and save the session.
   +      $has_session_manager = $this->container->initialized('session_manager');
   +      if ($has_session_manager) {
   +        $this->container->get('session_manager')->save();
   +      }
   

   FWIW, the issue title is misleading — you're not really removing persistence, all you're doing is to replace the 'persist' mechanism with some manual operations (that are executed in the same spot as the service persistence).

   I dislike that this hard-codes even more things into kernel/container (re)build process... what do we actually gain from that?

3. +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
   @@ -15,13 +15,6 @@
   -  public function initialize();
   

   Still part of the public interface + actively called, shouldn't be removed.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -108,14 +116,12 @@ public function __construct(RequestStack $request_stack, Connection $connection,
+    if (($this->started || $this->startedLazy) && !$this->closed) {
+      return TRUE;
+    }

@@ -125,7 +131,7 @@ public function initialize() {
+      $result = $this->start();

@@ -135,17 +141,26 @@ public function initialize() {
+      $result = FALSE;
...
-    return $this;
+    $this->startedLazy = TRUE;
+
+    return $result;

This looks odd to me - like it would you could call startLazy twice and the first time it returns FALSE and sets $this->startedLazy to TRUE and then the next time it returns TRUE even though a session has not been started.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -466,6 +479,12 @@ protected function initializeContainer() {
+      $this->container->get('session_manager')->startLazy();;
...
+      $this->container->get('session_manager')->start();;

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1107,7 +1120,12 @@ protected function rebuildContainer($environment = 'prod') {
+      $this->container->get('session_manager')->startLazy();;
...
+      $this->container->get('session_manager')->start();;

double ;

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -402,6 +402,7 @@ protected function initializeContainer() {
+    $session_manager_state = 0;

@@ -409,6 +410,18 @@ protected function initializeContainer() {
+          $session_manager_state |= 0x1;
...
+          $session_manager_state |= 0x2;

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1094,6 +1094,19 @@ protected function rebuildContainer($environment = 'prod') {
+    // If there is a session manager, close and save the session.
+    $session_manager_state = 0;
+    if (\Drupal::getContainer()->initialized('session_manager')) {
+      $session_manager = \Drupal::getContainer()->get('session_manager');
+      if ($session_manager->isStartedLazy()) {
+        $session_manager_state |= 0x1;
+      }
+      if ($session_manager->isStarted()) {
+        $session_manager_state |= 0x2;
+      }
+      $session_manager->save();
+      unset($session_manager);
+    }
 

@@ -1108,7 +1121,12 @@ protected function rebuildContainer($environment = 'prod') {
-    $this->container->get('current_user')->setAccount(\Drupal::currentUser());
+    if ($session_manager_state & 0x1) {
+      $this->container->get('session_manager')->startLazy();
+    }
+    if ($session_manager_state & 0x2) {
+      $this->container->get('session_manager')->start();
+    }
 

This feels wrong. What I think this code is saying is that the DrupalKernel is managing the state of the SessionManager. Which is putting too much responsibility on the DrupalKernel.

SessionManager should manage the state of the SessionManager.

As a sign that this should be extracted to the SessionManager is that the logic is duplicated in the WebTestBase.

Cool so this is an intermediate step? If so everything else looked good

Cool so this is an intermediate step? If so everything else looked good

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -409,6 +410,18 @@ protected function initializeContainer() {
+      // If there is a session manager, close and save the session.
+      if ($this->container->initialized('session_manager')) {
+        $session_manager = $this->container->get('session_manager');
+        if ($session_manager->isStartedLazy()) {
+          $session_manager_state |= 0x1;
+        }
+        if ($session_manager->isStarted()) {
+          $session_manager_state |= 0x2;
+        }
+        $session_manager->save();
+        unset($session_manager);
+      }

@@ -466,6 +479,12 @@ protected function initializeContainer() {
+    if ($session_manager_state & 0x1) {
+      $this->container->get('session_manager')->startLazy();
+    }
+    if ($session_manager_state & 0x2) {
+      $this->container->get('session_manager')->start();
+    }

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1094,6 +1094,19 @@ protected function rebuildContainer($environment = 'prod') {
+    // If there is a session manager, close and save the session.
+    $session_manager_state = 0;
+    if (\Drupal::getContainer()->initialized('session_manager')) {
+      $session_manager = \Drupal::getContainer()->get('session_manager');
+      if ($session_manager->isStartedLazy()) {
+        $session_manager_state |= 0x1;
+      }
+      if ($session_manager->isStarted()) {
+        $session_manager_state |= 0x2;
+      }
+      $session_manager->save();
+      unset($session_manager);
+    }

@@ -1108,7 +1121,12 @@ protected function rebuildContainer($environment = 'prod') {
+    if ($session_manager_state & 0x1) {
+      $this->container->get('session_manager')->startLazy();
+    }
+    if ($session_manager_state & 0x2) {
+      $this->container->get('session_manager')->start();
+    }

For my tastes there is way too much session manager logic going on here. Let's encapsulate this on the SessionManager.

@znerol looking #2285621: Remove persist-tag and replace it with a container.state_recorder service - I don't get the benefit of creating separate state recorders of the services that need persistent state - this looks overly complex. Can't we just have an interface on each service that needs state persisting with the methods getState() and restoreState().

I like the idea of closing the session and starting it again across container rebuilds, it's special after all.

I do wonder a bit whether we really need to maintain the exact state though - we have lazy session initialization for a reason, so if the session has been closed then started lazily again, it ought to just work if it gets accessed again after it's closed. That would save a lot of the logic here in the session manager.

I hadn't noticed that issue but it looks sensible. I'd be tempted to bump this issue to critical and that one to major?

The workaround here seems OK to me if we think we can remove it via that issue.

So how about we move forward with the patch in #27 to unblock the #2239969: Session of (UI) test runner leaks into web tests and a few other patches?

So, I agree with persisting objects being a problem, but I really do not like this approach. Our implementation is very complex and we're adding a new level of complexity. Specifically:

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -108,14 +116,39 @@ public function __construct(RequestStack $request_stack, Connection $connection,
+  public function getState() {
+    $state = 0;
+    if ($this->isStartedLazy()) {
+      $state |= static::STARTED_LAZY;
+    }
+    if ($this->isStarted()) {
+      $state |= static::STARTED;
+    }
+    return $state;
+  }

+++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
@@ -15,11 +15,50 @@
+  /**
+   * Gets the current state of the session manager.
+   *
+   * @return int
+   *   An integer representing the how the session is started. Either
+   *   self::STARTED_LAZY, self::STARTED or the two added together.
+   */
+  public function getState();

Having objects with state is hard and complicates how everything interacts with it. Every state has to be handled by the calling code which means we're moving directly _away_ from Symfony's session objects and into a stand alone system. SessionStorage already has state as well in SessionStorageInterface::isStarted() so we're either adding a second state or just saying "that state isn't valid, you have to use our method" which again has the previous problem of us moving away from the interface's we're trying to move towards.

In my opinion #17 is just as bad - hard coding state assumptions about the SessionManager in WebTestBase and DrupalKernel? These have no business maintaining that. So we're still maintaining a second state....

The goal has to be to completely remove state and service persistence so if #17 is going to get us there quicker then let's go with that. Actually neither patch moves us nearer or further imho this just unblocks patches that might eventually get us there i.e. the removal all other service persistence, #2239969: Session of (UI) test runner leaks into web tests and #2284103: Remove the request from the container

I think container rebuilding mid request is a mistake - as @fabpot says after a container rebuild we should just do a new request - therefore no need to maintain the request stack or session - but doing that now is going to cause us heaps of pain.

As I said let's get #17 in and see if we can inch towards maintaining a minimal state or selected services across rebuilds.

rtbc for #17

Fine with doing #17 (I'm actually fine with either as long as we get the issues following this one done).

I don't like either of these personally and see it as a leap backwards in terms of complexity we have to refactor to really fix the problem.

At least lets rebase this on #2016629: Refactor bootstrap to better utilize the kernel which changes the kernel methods we're touching similar to #2281903: Refactor DrupalKernel::initializeContainer in preparation of a replacement for persist services which is related to this issue.

Yeah I noticed that the hunk in WebTestBase would not be needed - which is great and a sign we're on the right path albeit it seems a little windy :)

Thanks @znerol for sticking with this!

@neclimdul, I don't think it's complexity we'll need to refactor, it's more a case of temporary hacks we can just delete once it's working properly.

Probably we should introduce a StatefulService interface and let services implement their own persisting logic at some point.

I think that would be a much better idea.

interface StatefulServiceContainerRebuildInterface {

  public function saveState();

  public function restoreState($backup);
}

Would help to eliminate these fugly lines:

+    if ($session_manager_state & 0x1) {
+      $this->container->get('session_manager')->startLazy();
+    }
+    if ($session_manager_state & 0x2) {
+      $this->container->get('session_manager')->start();
+    }

…and have something along the lines of the following instead:

class SessionManager implements StatefulServiceContainerRebuildInterface {

  const IS_STARTED_LAZY = 0x1;
  const IS_STARTED = 0x2;

  public function saveState() {
    if ($this->isStarted()) {
      $this->write();
      return static::IS_STARTED;
    }
    return static::IS_STARTED_LAZY;
  }

  public function restoreState($backup) {
    $backup & static::IS_STARTED ? $this->start() : $this->startLazy();
  }
}

I'm primarily concerned that this patch is hard-coding this facility for selected core services without giving (core + contrib) modules any way to do the same.

@sun - did you see #30/#31?

1. The session is unique (or almost unique) in terms of needing to maintaining state at the moment. We specifically do not want any services to do this at all.

2. Even session shouldn't need to maintain state if we can close and open it for anonymous users without throwing an exception due to session ID issues.

Sorry, it wasn't my intention to hold up this patch. Let's move forward here.

Committed/pushed to 8.x, thanks!

This change caused the following fatal error to appear when hitting rebuild.php:

Fatal error: Call to a member function isAnonymous() on a non-object
  in \core\lib\Drupal\Core\Session\SessionManager.php on line 197

Call Stack
#	Time	Memory	Function	Location
1	0.0010	133272	{main}( )	..\rebuild.php:0
2	0.0250	1303400	drupal_rebuild( )	..\rebuild.php:37
3	3.7432	11490224	drupal_flush_all_caches( )	..\utility.inc:68
4	5.5933	17705416	Drupal\Core\DrupalKernel->updateModules( )	..\common.inc:4099
5	5.5933	17702952	Drupal\Core\DrupalKernel->initializeContainer( )	..\DrupalKernel.php:647
6	5.5933	17703048	Drupal\Core\Session\SessionManager->save( )	..\DrupalKernel.php:707

Attached patch fixes the problem.

Any chance to get this quick follow-up in? Would love to use rebuild.php without fatal errors again.

Committed b314a3e and pushed to 8.x. Thanks!