|Summary:||mod_session_dbd should not save the session at each request|
|Product:||Apache httpd-2||Reporter:||Christoph Rabel <christoph.rabel>|
|Component:||mod_session_dbd||Assignee:||Apache HTTPD Bugs Mailing List <bugs>|
Proposed new directive SessionExpiryUpdateInterval
Updated tests for proposed patch
Description Christoph Rabel 2014-12-02 22:55:13 UTC
Whenever a session is fetched from the database it is immediately saved again: (excerpt for a single request from mysql logfile) 91 Execute select value from Session where sessionkey = '1fad7788-c769-4872-aa77-00f1c7e95e50' and (expiry = 0 or expiry > 1417557643821571) 91 Execute update Session set value = '...', expiry = 1417558643858512, sessionkey = '1fad7788-c769-4872-aa77-00f1c7e95e50' where sessionkey = '1fad7788-c769-4872-aa77-00f1c7e95e50' While I understand that the expiry time has to be updated, is it really necessary to do it for _each_ request? This is especially annoying since a "Cache-Control no-cache" header is set and the update is done for each image, js, css, ... I propose to add a new option to allow to specify a "no-update-period-time" based on expiry. Basically something like: if (expiry + no-update-period-time < now) session_dbd_save(...); While this will introduce some fuzziness to session expiry time, it usually doesn't matter if the session expires a couple of seconds or minutes earlier. Please see also: https://issues.apache.org/bugzilla/show_bug.cgi?id=57299
Comment 1 Paul Spangler 2015-08-19 19:21:30 UTC
Created attachment 33017 [details] Proposed new directive SessionExpiryUpdateInterval This patch adds a new directive to mod_session, SessionExpiryUpdateInterval, similar to the description by Christoph. It defines the number of seconds that the expiry value may change in a clean session without having to save the session again. It's added to mod_session instead of mod_session_dbd because the session submodules don't have access to the original expiry value. It also has the benefit of skipping session encoding/crypto. This change is disabled by default, meaning sessions with expiry changes will be written every request unless this directive is set. By putting this in mod_session, it means submodules won't always receive a call to save. So the Cache-Control headers must be set on session load, which I would argue is more correct anyway, since the use of session data in processing the request (e.g. form auth) should prevent caching, regardless of whether or not the response contains session data. The patch also includes a change to how sessions without a max age are saved. Following the same logic that we don't need to write a session if the data hasn't changed and the expiry has only changed by a small amount, we don't need to write a session if the data hasn't changed and there is no expiry at all. In this case, it's a behavior change that is not controlled by a directive. Perhaps the most noticeable effect is that new sessions that are never written to are never saved. For example, the presence of Session On without anything that uses the session will no longer result in a session cookie. I will attach a second patch containing the test changes.
Comment 2 Paul Spangler 2015-08-19 19:28:47 UTC
Created attachment 33018 [details] Updated tests for proposed patch Test changes for the SessionExpiryUpdateInterval. It adds new tests that are skipped in 2.4 for now, and updates existing tests that do not modify the session to expect that the session will not be saved. Those tests are marked todo in 2.4 since they will fail without the changes in the proposed patch. All tests pass against 2.4 without the patch and trunk with the patch. In my manual tests using mod_session_dbd with sqlite, I observed a 10-20x improvement in time required to make 600 requests that use mod_auth_form. That's essentially a worst-case scenario since sqlite is not optimized for writes. I also observed a MySQL installation where the same 600 requests no longer generated 600 UPDATE queries to the DB server.
Comment 3 Paul Spangler 2015-08-21 16:03:23 UTC
Just to be explicit about it given the size of the patches: The code in this patchset is Copyright (c) National Instruments, licensed to the public under the Apache License 2.0.
Comment 4 Yann Ylavic 2015-10-16 22:40:02 UTC
Thanks Paul, committed in r1709121. I will let others review it on trunk, commit and run the tests, and then propose a backport.
Comment 5 Yann Ylavic 2015-10-16 22:47:26 UTC
(In reply to Yann Ylavic from comment #4) > Thanks Paul, committed in r1709121. Forgot to mention that I made minor changes, like s/long/apr_time_t/ for expiry_update_time or SessionExpiryUpdateInterval directive doc/command description and validation.
Comment 6 Paul Spangler 2015-10-16 22:56:10 UTC
(In reply to Yann Ylavic from comment #5) > (In reply to Yann Ylavic from comment #4) > > Thanks Paul, committed in r1709121. > Forgot to mention that I made minor changes, like s/long/apr_time_t/ for > expiry_update_time or SessionExpiryUpdateInterval directive doc/command > description and validation. Sounds good. Thanks for taking a look at it Yann!
Comment 7 Paul Spangler 2015-10-19 15:44:09 UTC
Created attachment 33187 [details] Updated tests Tests updated to match changes made in r1707969 for handling large numbers. Now applies cleanly to httpd test trunk.