If for any reason a Redis session lock is not being released, all subsequent
requests will wait to acquire the lock, forcing them to time out eventually.
This will happen till the original lock finally expires after the session timeout.
This sets the Redis session lock expiry time to whatever is lower,
either the PHP execution time `max_execution_time`, if the value was
defined in the `php.ini` or the globally configured `sessiontimeout`.
Setting it to the lower of the two will not make things worse it if the
execution timeout is longer than the session timeout.
For the PHP execution time, once the PHP execution time is over, we can
be sure that the lock is no longer actively held so that the lock can
expire safely. Although at `lib/classes/php_time_limit.php::raise(int)`,
Moodle can progressively increase the maximum PHP execution time, this
is limited to the `max_execution_time` value defined in the `php.ini`.
For the session timeout, we assume it is safe to consider the lock to
expire once the session itself expires.
If we unnecessarily hold the lock any longer, it blocks other session
requests.
Co-authored-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
A snapshot of the session is now taken when write_close is called.
The session at shutdown is then compared to the snapshot. If changes
are detected, they are logged. This aids developers in seeing if
early session closes may be having unintended consequences.
Previously, newly added keys to the session were not detected. Objects
with the same properties were also incorrectly reported as different.
This commit improves this, and updates the unit tests to reflect the
new functionality.
PHP before version 8.1 automatically converted to int if the function
parameter (or array key) is expected to be int. PHP 8.1 shows notice in
this case
The Redis cache store and session handler both do a 'ping()' after
connecting to Redis. This is unnecessary because the connect() call
has just checked the network connection and it's hardly likely that
the server has gone down since then.
According to my profiling, both connect() and ping() take
measurable time when talking to a separate server, i.e. a few
milliseconds. So it's not the case that connect() doesn't really
talk to the server, as I initially wondered.
If using Redis on a separate (non-localhost) server for both session
and cache store, removing these ping calls can save a millisecond
or two per request.
The 'unable to obtain session lock'-exception raised by the Redis
session handler is hardcoded in English and not all that useful
to the end user.
This change adds the error message to the lang/error.php and gives
the user further hints why the error might have occured and how it
could be fixed.
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
The PHP_CodeSniffer @codingStandardsIgnore annotations are deprecated
and, since version 3.x, the new // phpcs:ignore comments should be used
instead.
This commits just reviews all the uses in core, replacing them for
the better new candidate, or removing when no longer needed.
This commit removes code that only was being executed by php < 73
and it's 100% safe to do so because Moodle 3.11 and up require
php 73, hence it was not executed ever.
Removed code includes:
- ldap_control_paged_result and ldap_control_paged_result_response
(that were deprecated in php 73 and have been removed in php 80).
- conditional code in the session manager, where some hacks were
needed for php < 73. Note that this removes the private function
append_samesite_cookie_attribute() completely because it was
doinf nothing (first line was returning for php < 73).
- Also removed the old session.hash_function ini setting because
it was removed in php 71.
Kept code includes:
- The environmental check_igbinary322_version test has not been
removed because it doesn't hurt (always returns "ok" for php 73
sites) and doing it would involve to backport the environment.xml
file to 39 and 310. Instead, a note has been added to MDL-71747
in order to get rid of that check for 4.1 and up.
When session lock debugging and read only sessions deubgging are both
enabled, session lock debugging becomes moot. This patch causes
the session lock deubgging code to exit early if read only sessions
debugging is enabled.
The random retry delay for redis session cache was calculated as
rand(100000, 500000) giving an effective retry delay of 100 seconds
to 500 seconds. That's off by a factor of a thousand! Using Redis as a
session cache and when the connection hangs, you can get random
"cannot obtain session lock" errors because it's waiting up to
500 seconds (or about 8.33 minutes) for a Redis connection.
This sets the delay to the originally intended 100ms to 500ms.
(see MDL-59866).
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Prior to this patch the debugging mode (when enabled) would trigger
on everywhere, regardless of whether or not READ_ONLY_SESSION is defined.
This patch modifies that behaviour so that the debugging only kicks in
if READ_ONLY_SESSION is defined and set to true.
If a developer has debugging on then they will recieve all debugging
messages for locking whether wanted or not. We already have a setting
to display these messages.
In 39770792ca read-only sessions were allowed.
In the redis case, as called from the mobile application,
this can lead to returning 'false' for session rather than ''.
CSRF protection for the login form. The authenticate_user_login function was
extended to validate the token (in \core\session\manager) but by default it
does not perform the extra validation. Existing uses of this function from
auth plugins and features like "change password" will continue to work without
changes. New config value $CFG->disablelogintoken can bypass this check.
