-
Notifications
You must be signed in to change notification settings - Fork 11.5k
Description
- Laravel Version: 6.9.0
- PHP Version: 7.3.13
- Database Driver & Version: Redis
Description:
Since Laravel persists the entire session data array at the end every request, in the case of concurrent requests, any changed session data is very likely to be lost.
framework/src/Illuminate/Session/Middleware/StartSession.php
Lines 62 to 65 in 50e43a5
// Again, if the session has been configured we will need to close out the session | |
// so that the attributes may be persisted to some storage medium. We will also | |
// add the session identifier cookie to the application response headers now. | |
$this->saveSession($request); |
Related issues:
- Laravel 5.0 - Asyncronous AJAX Requests cause Session Variable Changes to be Overwritten #7549
- Laravel Session OverWriting Every Time On Each Request #28147
- [5.1] Concurrent requests wiping out the session #14385
- Missing session keys on concurrent ajax requests #14162
Proposed fixes:
- Don't save the session in the
terminate()
method. [Proposal] Session save timing ideas#1161. This was changed in 5.8 with this PR: [5.8] Fixes session attributes persistence #26410. But that change doesn't fix this issue. - Only save data to session that is actually changed. Store session data per key #18187
- Use session locking. https://github.com/rairlie/laravel-locking-session
- Check whether session data has changed before saving it. [6.0] alleviate session race condition #29410
Steps To Reproduce:
In the below example of concurrent requests, the change in Request A will be lost, overridden when StartSession::saveSession($request)
is called at the end of Request B's lifecycle:
|-------- Request A (changes session variable) --------|
|------------ Request B (no changes to session data) ------------|
Recommendation:
The only complete solution to this problem is to use session locking. A package like this one will work: https://github.com/rairlie/laravel-locking-session. However, that basically makes all your AJAX requests synchronous, which isn't a great solution for modern SPA's.
@taylorotwell has mentioned some form of locking as a solution to the problem in #7549 (comment).
However, I do think we can make other improvements. The fact is, most AJAX requests in a modern SPA aren't modifying session data. Nevertheless, they persist the session data at the end of their lifecycle. This means that a single long-running request makes the session data unable to be changed reliably.
After looking at all these related issues and proposed fixes, I think something like this PR #29410 is the most tenable. It basically only persists the session data if it is dirty. It probably could be altered to be enabled/disable through a config option. It doesn't solve the problem if concurrent requests all want to alter the session data, but it does solve the 90% use case where most requests are just reading session data. The one issue I foresee with this approach is session timeout.