GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Simple concurrency solution for native sessions based on discussions in #1746. Allows closing current session with $this->session->native->sess_close() to release file lock and allow session access to concurrent requests. Also offers limited-time forwarding of old sessions to newly regenerated session IDs so concurrent requests don't lose access to current session data when ID is regenerated. Forwarding enabled by $config['sess_forward_window'] = <int>;.
$config['sess_forward_window'] = <int>;
Credit to @GDmac and @Areson for contributions to this solution.
Signed-off-by: dchill42 firstname.lastname@example.org
@GDmac and @Areson, here is the new solution we discussed for Native. It needs proper testing - I have only verified that unit tests still pass.
While playing with this implementation, I realized that after session_write_close() read and write access is still available to $_SESSION. The read access may be a nice convenience, but the write access is misleading in that new values are NOT stored to actual session data. I'm thinking we may need to trigger an error on write after close to help devs realize they are out of sync. It's not terribly hard to do, but I'd like your feedback before implementing.
[Edit] Also, do you think there is a need or use for a reopen() function to restore write access after closing? It seemed somewhat pointless to me, but I could see an argument for multiple brief critical sections within a request to facilitate more complex concurrence.
that last bit (reopen) would reserve for a custom library.
you already have to deal with that kind of stuff in the hmvc layer,
for where other controllers might rely on session.
Native sessions implement file locking, which should resolve concurrency problems. Have you ever experienced this problem that you're fixing?
@narfbg Yes, the default file handler for native sessions does use a file lock to prevent concurrent access to the session data. That maintains the integrity of the session data, but it also forces concurrent requests to be sequential. There are scenarios in which this actually creates problems for efficiently handling the requests (especially when using AJAX).
The sess_close() function added to this driver (but not the Session library at large) gives the developer the opportunity to make changes to session data and then release that file lock so other requests can access the session while the first request continues to do other processing. This supports better overlapping of concurrent requests, especially when a long request overlaps with a very short one (or more). Usage is completely optional.
The forwarding feature is enabled by setting the new configuration variable, also making it completely optional. When set to a positive number of seconds, it engages a mechanism that allows concurrent requests to "catch up" with a regenerated session ID. If the client sends a request which will trigger an ID regen, and another request is initiated before the new ID is returned to the client, the second one will try to access an orphaned session with the old ID, ruining the session state maintained in the data. This feature allows the old session to be "forwarded" to the new session for a brief (developer specified) window in order to alleviate that problem.
These are issues that have been tested and verified, and there is a certain amount of demand for a solution. You have already seen a handful of requests related to these problems, such as #154, #1283, #1713, #1900, and probably others. There is also a very well-written article which explains the trouble in detail and compares a variety of solutions.
Added close and forwarding features to native Session
Signed-off-by: dchill42 <email@example.com>
I've run my battery of tests for session forwarding and it looks like we need a few tweaks to get it operating as expected. I'll make a couple of comments on the commit on the specifics I noticed.
Just a quick apology for all of the comment spam. All in all it looks pretty good, and it's a nice and simple implementation. Great job so far @dchill42.
Don't call sess_destroy() here. We lose our now correct cookie and our $_SESSION data for future requests coming in with the old session id that still need forwarding. We can call a session_write_close in the if statement below.
In this condition, the current session has the old session ID and the session data contains only the forwarding data. We want that cookie to be replaced with the new value. Did you encounter a problem with the forwarding during testing? Perhaps I overlooked something, but I believe the general logic here is correct.
I did run into a problem during forwarding. If we have multiple requests that need to be forwarded, they all rely on having access to the session data for the old session id. If we destroy it, the forwarding will fail for all but the first request that we forward. That's why I suggested removing this delete. This does mean that we will have to rely on the built-in GC for native sessions to clean up the old data.
Oh! It's wiping out the forwarding data, isn't it? Oops - that was not the intended effect, I'll fix that.
Add a call to session_id($new_id) here to make sure we really start the new session. This will cause a new cookie to drop, but that is fine as long as our sess_time_to_update window is larger than our sess_forward_window. Also, if we have any flags/values used with the old session id, we will need to unset them here before starting the new session, or they get carried over.
The session_id() call is probably a better solution than setting $_COOKIE directly. I'll change that.
We can add an else here to destroy the session safely if we get a request after the window has close. Note that calling sess_destroy rather than session_destroy() will cause any cookie with a valid session id to go away, which means dropped sessions. This may be the behavior we want.
We want to protect this section from being called by an old session id once the session has been regenerated. We can check for the presence of $_SESSION['sess_new_id'] and prevent code from running if it is set as long as we unset it above when forwarding takes place (otherwise it remains set on the new session id).
We should update the $_SESSION['last_activity'] value here.
Superseded by #3073.