SaveSessionHandler saves the session too early when using a StreamedResponse

[bestform]

In 9c1e468 a new response listener was introduced by @Tobion , that saves the session on a kernel response event. When using a streamed response, this is actually pretty early, as most of the code is handled in the callback, that is being called after the response event was fired.

We ran into a problem with this setup as we used "isStarted" on the session instance, which of course is false after said listener has done its work.

It is argued in the commit message, that saving the session at an arbitrary point in time isn't critical, as it will be started again on demand. In the case of a StreamedResponse this leads to starting the session twice on every request. Moreover the return value of isStarted() on the session instance - while being technically correct - is quite misleading, as the session has in fact been started already and is just waiting to be restarted on demand after closing it via the save() call in the new listener.

I don't know a good solution for this from the top of my head but I would like to see it being discussed further, as the situation, that arises when using a StreamedResponse, isn't ideal at all.

[Tobion]

You don't have to open the session twice when using streamed responses if you do you session stuff in the controller and not in the streamed response callable. And then just use the session information you read in the controller inside the streamed response.

[bestform]

Thanks for your quick reply. I see your point.

Unfortunately our layout has way more logic inside the callable. To only wrap the rendering part inside the callable would mean, that we execute the complete controller logic during the handleRaw method in the HttpKernel. That would minimize the desired effect of streaming html the moment it is ready.

Granted this is due to our special case, but I guess it is not too exotic, as just splitting of the rendering part into the closure wouldn't be that much of a speed increase for the user.

To be more specific: in our case a request lands in a special main controller, which in turn renders some modules inside a user defined layout using sub requests. By putting this logic inside the closure, every module will be streamed to the client as soon as it has been rendered by the main controller.

Maybe our layout just doesn't fit with the intended use of the StreamedResponse class, but it would be a shame to drop it, because the user experience benefits immensely from this way of streaming the response to the client.

Do you have any idea how one could tackle this problem? In a vanilla symfony project using the StreamedResponse, is it common to just do the rendering in the callable and everything else in the action of the controller? in my opinion this wouldn't be such a great speed increase on the client side.

(when I am talking about speed increase I am of course talking about a perceived improvement, not an actual one. But it makes a huge difference)

[Tobion]

I see your point. I think there are 3 ways of dealing with this:

1. execute the save session after streaming the response. This will result in the concurrency problems again for people that are not careful that we tried to fix, see doc of the listener. E.g. users log in and devs send a streamed response wihout saving the session first can result in session loss.
2. You move all session writing into the controller and session reading in the callable should not restart the session as I suggested in Checking whether there is data in the session, creates a session #6036 (comment) This way, the session is normally not started twice on every request.
3. Leave as-is and live with the session restart. How strong is the impact for you (performance wise)?

[bestform]

Restarting the session isn't a performance issue, but i'd rather not do it, just to nullify a symfony feature. I'd rather adapt to symfony and reevaluate how we utilize the StreamedResponse.

Thank you very much for your input. For the moment being, we will probably just eliminate the new listener in a compiler pass, so that current code, that relies on session::isStarted being true, stays happy. We know about the potential problems the new listener tries to solve and will look out for options to include it again in the future.

Would it be an option to make the SaveSessionHandler optional via a config parameter, so you don't have to hack it away using a compiler pass? I know this is a very special use case, so i'd understand if you decide to not clutter the configuration options with this, but maybe it's worth a consideration.

[msumme]

Session reading causes the problem as well.

// From Session.php
**
     * {@inheritdoc}
     */
    public function has($name)
    {
        return $this->storage->getBag($this->attributeName)->has($name);
    }

    /**
     * {@inheritdoc}
     */
    public function get($name, $default = null)
    {
        return $this->storage->getBag($this->attributeName)->get($name, $default);
    }

//From NativeSessionStorage.php

/**
     * {@inheritdoc}
     */
    public function getBag($name)
    {
        if (!isset($this->bags[$name])) {
            throw new \InvalidArgumentException(sprintf('The SessionBagInterface %s is not registered.', $name));
        }

        if ($this->saveHandler->isActive() && !$this->started) {
            $this->loadSession();
        } elseif (!$this->started) {
            $this->start();
        }

        return $this->bags[$name];
    }

Any idea how to work around this?

[msumme]

I'm actually getting an issue where because the session is opened + closed we get a 500 "Failed to start the session: already started by PHP ($_SESSION is set). "

I'm not sure what the correct way to fix that is... but it also had to do with using a StreamedResponse.

[kbond]

@msumme, to clarify, you are getting this error when not using a StreamedResponse as well? I have reported the issue here: #12562

[msumme]

@kbond no. I start getting the issue w/ StreamedResponse after upgrading to SF 2.5.7 because of the new listener behavior.

I haven't seen it outside of that, but it is in fact related to the new listener which I confirmed by overriding the listener service definition to point to an empty class.

[kbond]

Ok, my issue was also fixed when I did the same.

[bestform]

Same here. Removing the listener fixed our problems. But I am not happy with this solution of course. This should only be necessary temporarily.

[mcrio]

I'm doing a sub-request (->forward) within the StreamedResponse callback, so the Session is failing to start: Failed to start session because headers already sent by..

One solution was to override the introduced save session listener. Any other workaround? Problem is that the session data is required in the sub-request.

[nicolas-grekas]

I'm closing here because the issue is old, and the current behavior looks sensible to me: reading/writing to the session should be done before sending the first byte. Streamed responses need to be built around this IMHO. This also ensures that session is not locked for too long.

[AndrolGenhald]

@nicolas-grekas This makes sense to me, but the documentation for doing a legacy route loader suggests using a StreamedResponse, which causes issues when using the session in the legacy code. Should this be reopened, or should that documentation be fixed with an alternative suggested implementation?

Edit: Perhaps it's a non-issue, I think the problem is that I removed the session_start() call in the legacy code when I did the Legacy Bridge, but it needed to be added back when migrating to the Legacy Route Loader.

[nicolas-grekas]

Please send a doc PR yes if you think some details are missing.