Repository navigation
Conversation
c651024 to
2b96d11
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Okay but this is a behaviour change though, so deferring the final approval to an ext/session codeowner
|
Agreed, may even need to rebase to just 8.6+, but it is a bug so I'll let the code owner make the final call here. |
|
I am not sure this should be merged now. It changes the semantic of the failed write. If first session write failed, why it should be retried? There is bigger chance that it will fail again than succeed. Moreover, the flow does not know whether nothing was written, the write partially completed, or the external operation already succeeded before the failure. I think firstly we should explicitly decide the semantics of a bailed-out save-handler invocation, especially whether write() may be retried, because this fix exposes this behavior. I will try investigating this issue.
You have basically co-created this module, so you are code owner too ;) |
| self::$writes++; | ||
| if (self::$writes === 1) { | ||
| /* Force a real bailout (fatal OOM), not an unwind-exit */ | ||
| $GLOBALS['oom'] = str_repeat('a', 64 * 1024 * 1024); | ||
| } | ||
| echo "write ", self::$writes, "\n"; |
There was a problem hiding this comment.
Just use zend_trigger_bailout from the zend_test extension.
There was a problem hiding this comment.
Done. The PR now targets PHP-8.6.
| { | ||
| int i; | ||
| bool bailout = false; | ||
| if (PS(in_save_handler)) { |
There was a problem hiding this comment.
Can't you just check for if (UNEXPECTED(EG(bailout)) and return with an UNDEF retval?
There was a problem hiding this comment.
Or even EG(active) maybe? But I guess we're still active while calling it during shutdown.
There was a problem hiding this comment.
EG(bailout) is set by every zend_try, so it is non-NULL during the shutdown flush regardless. CG(unclean_shutdown) is set by any fatal, including one outside the handler, where the session is still written at shutdown today; keying on it would drop that write.
2b96d11 to
5f717e2
Compare
iliaal
changed the title
|
Agreed. Reworked so that after a bailout in a handler or the flush, later flushes only call close(): nothing is written again and session_regenerate_id() refuses, but the handler can still release its lock. Retargeted to PHP-8.6 since this changes behavior. |
|
Is this still PR (and linked issue) relevant now that we have the general graceful OOM handling? |
5f717e2 to
628f127
Compare
|
The OOM case from the issue is gone on PHP-8.6. Any other fatal inside write() still hits it, e.g. max_execution_time or a "Cannot redeclare function" from eval(): the shutdown flush warns "Cannot call session save handler in a recursive manner" and reports the write as failed. So the PR still applies, but GH-15529 itself is fixed. |
…lout A zend_bailout() from inside a userland save handler, such as a fatal error in write(), left PS(in_save_handler) set, so the next handler call was refused with "Cannot call session save handler in a recursive manner": request shutdown retried the failed write(), or, when the bailout came from the shutdown flush itself, close() never ran. Clear the flag once the handler returns or bails out, and record the bailout, as well as one from php_session_flush() itself. While it is recorded, a flush only closes the handler, so neither a failed encode or write() nor the empty data left by a bailed-out read() is written, and session_regenerate_id() refuses. A bailout in open() keeps an already open session active so it can still be closed. Initializing the session clears the record.
628f127 to
cd92546
Compare
A bailout from inside a userland save handler, a fatal error inside write() for instance, left PS(in_save_handler) set, so the next handler call was refused with "Cannot call session save handler in a recursive manner": request shutdown retried the failed write(), or, when the bailout came from the shutdown flush itself, close() never ran. The flag is now cleared once the handler returns or bails out. The bailout is recorded, as is one from the flush itself, and while it is, a flush only closes the handler and session_regenerate_id() refuses, so a failed write() is not retried and the empty data left by a bailed-out read() is never written over the stored session. A bailout in open() during session_reset() keeps the already open session active so it is still closed. bug60634_error_3.phpt and bug60634_error_4.phpt asserted the old warning and now assert the close handler running.