Fixes for Laravel Session, reset between requests

[lombax85]

I make this pull request to complete the work done by duxet 1 year ago on pull request #39 .
I tried to implement his work but unfortunately the issue with auth persists. Specifically, when I login using standard Laravel authentication, if I open another browser (cookie cleared, clean session), I am directly logged in.
It seems that this happens because the standard Laravel Session is a singleton, and persists between requests. I adapted #39 code and added an extension to make session.storage no more singleton, and now the session is reloaded at each request.
This has been tested, by now, with Laravel 5.3.

[marcj]

why not instanceof?

[lombax85]

For this specifical part I copied the implementation from #39 and checked it with xdebug that it was working correctly. If for some reason you prefer instanceof (they should be equivalent in the effects, but probably instanceof is faster since is a language construct and not a function) I can test it in the next few days and update the implementation

[lombax85]

Ok, I had one hour free and tested/updated the code

[andig]

Please check the indents, too.

[lombax85]

my bad, something broken in phpstorm (??) didn't notice the tabs. Fixed

[lombax85]

I had the chance to do a load test and unfortunately it seems that this fix doesn't work well on the performance side:

ab -c 10 -n 1000 http://127.0.0.1/

Php 7.0.18 and simple test page

Apache+Php: 500 req/sec
Php-pm without fix: 1.500 req/sec
Php-pm with fix: 300 req/sec

Since I'd like to make this fix work (the actual version of the httpkernel cannot be used in a Laravel application with authentication by now, because of the bug with sessions), do you have any suggestion or contribution to help analysing performance? my usual tools are Xdebug + profiler + webgrind, but at a first try it seems that profiling an "always on" application with webgrind is not so easy as a standard php webapp (a lot of event-related stuff that hides the remaining code). Maybe you have something more effective to suggest :-)

[andig]

Apache+Php: 500 req/sec
Php-pm with fix: 300 req/sec

That's funny. It is of course expected that initializing the session comes with a penalty but the same should happen with Apache.

I have no clue about Laravel but apparently the implementation is resetting more than need be done and therefore incurring higher setup costs than apache?

[lombax85]

I think that this is because this change:

$this->app->extend('session.store', function() use ($app) {
 		    $manager = $app['session'];
 		    return $manager->driver();
 	    });

by default the session.store in Laravel is declared as a singleton, so instanced once for each http request and then reused each time.
The way Laravel handles singleton doesn't work with php-pm, since it expects that when the request ends, all the static are destroyed.
With my override I rebuild the session from scratch every time a part of the app references it (via DI Container or via app('session') ). However, during a single request, this can cause multiple (potentially "infinite") hits to the storage, one for each reference to the Session.
If this is confirmed, I have to change a little the behavior to use it as a singleton only during the current request, or better to persist it only through requests made by the same user (something like caching all sessions in a KV array, with $k == 'session_id' and $v == Session instance)...
I'll work on it in the next days

[andig]

With my override I rebuild the session from scratch every time a part of the app references it (via DI Container or via app('session') ). However, during a single request, this can cause multiple (potentially "infinite") hits to the storage, one for each reference to the Session.

I see. So- during a single request- you're essentially recreating the session store with each access.

... or better to persist it only through requests made by the same user (something like caching all sessions in a KV array, with $k == 'session_id' and $v == Session instance)...

Better not add guessing logic here- imho too cumbersome and unstable.

You could try to- instead of recreating the store on each access- to create it only on each request. The requests are handled by PHPPM\Bridges\HttpKernel. We could implement something like this to notify the Laravel bridge whenever a new requests starts in HttpKernel::onRequest() :

    if ($this->bootstrap instanceof RequestNotifierInterface) {
        $this->application = $this->bootstrap->notifyRequest($symfonyRequest, 'pre');
    }

You could then recreate the session handler only once per request.

Does this suggestion help?

[lombax85]

This could be a good suggestion, yes. However I want to do some more investigation because I'm not really sure that the problem with performance is because of that piece of code...by now it's only a supposition :-) I'll investigate further and let you know, thanks

[lombax85]

Ok, I had the chance to do some other test, and the slowness with php-pm was only a coincidence.
The slowness comes even with apache/php, it's related to Laravel.

I am doing my test using the local session storage (./storage/framework/sessions). At each request a file is added to the folder, but never removed/cleaned. This happens with apache-php or php-pm in the same way, it's Laravel behavior/bug.

After a lot of tests with ab, the application slowed down when listing the content of the folder for the high number of files present and during session Garbage Collection (see this file: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Session/FileSessionHandler.php#L101 ). So my previous tests were not reliable.

I made the tests again doing a cleanup before each session:

Apache+Php-fpm: 500 req/sec
Php-pm without PR: 1.500 req/sec
Php-pm with PR: 1.100 req/sec

Now:

• php-pm is always faster than Apache+php-fpm
• performances with my PR are lower than without, but this is normal (the standard httpkernel overperforms because it never reads the session)

Before asking you to evaluate and possibly merge this PR, I would only have a few days to try the fix you suggested (load the session only once per request), test if it makes my PR perform even better and maybe test it again using redis for storing sessions.

Thanks
Fabio

[cmizzi]

Is this PR still alive ?

[andig]

Can I ask you to fix the remaining conflict? Otherwise lgtm!

[cmizzi]

Contributor is un-active since Jun 2017... Is there's a way to push a patch on this PR without loosing author ?

[andig]

You can fork his repo/branch and take it from there. Viele Grüße, Andreas

…

Am 24.01.2018 um 14:11 schrieb Cyril Mizzi ***@***.***>: Contributor is un-active since Jun 2017... Is there's a way to push a patch on this PR without loosing author ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[cmizzi]

I've just prepared commits to be merged. I didn't success to push update on this PR so I created one (see referenced PR). I've fixed indent and rebased on master branch

[andig]

Great! Will take a look later this werk.

[andig]

Closed via #85