Re: [RFC] Revert/extend/postpone original RFC about read_only, lazy_write sessions

Hi Andrey, I've finished modifying patch, so I read your RFC. On Sun, Mar 16, 2014 at 3:24 AM, Andrey Andreev <narf@devilix.net> wrote: > I'm announcing the following RFC for discussion, with the hope that it > can get through before the PHP 5.6 release: > https://wiki.php.net/rfc/session-read_only-lazy_write > > As noted in it, I don't feel like > https://wiki.php.net/rfc/session-lock-ini was handled properly. > Lack > of attention to it alone is demonstrated by the fact that a total of > only 10 people have voted. I hope that this follow-up receives more > attention, so that we can avoid a potential mess. > // $_SESSION['logged_in'] has been set in a previous request Request 1: session_start(['read_only' => TRUE]); Request 2: session_start(); unset($_SESSION['logged_in']); session_write_close(); Request 1: if ($_SESSION['logged_in']) { /* logic */ } // evaluates to TRUE Your example ends up with the same result regardless of "read_only". If Request 1 locks session data, it finishes execution as logged_in==TRUE. There may be race condition like this. However, it does not matter much if legitimate users' requests are processed as authenticated or not under such race condition. It's _legitimate_ users' request anyway. If apps must not allow such race condition, any of session_write_close/commit(), read_only should not be used. (Request serialization is not perfect still, though. Please read session_regenerate_id() thread for details.) 2. The RFC describes that with 'lazy_write' set to TRUE, if no change was made to session data, then PS_UPDATE_FUNC() will be called instead of PS_WRITE_FUNC(). However, it only talks about internal implementation and fails to address (or at least mention) that a custom session handler should now also have an update() method. This is not correct. User defined save handler does not have to implement update() method. Session module is designed to allow omitting new method/function just like undocumented create_sid() function/method. There are test cases for it. (BTW, I noticed that I missed to compare dummy function address. It's fixed in my repository already.) "lazy_write" could be INI option and default to false, but it makes little sense because it is compatible with current behavior. I cannot think of good reason to disable it, but it may be disabled if you need. I don't see good reason to postpone this change still. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net