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

Hi, On Sun, Mar 16, 2014 at 7:32 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > // $_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. Read the explanation in my previous mail: http://marc.info/?l=php-internals&m=139498773308515 > 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.) It's about making a conscious choice of when to use it, not if at all. The name, and it's presence as an option instead of a separate function is misleading. > 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.) It's not incorrect, the RFC doesn't mention this case and what it happens in it. I admit, I haven't looked at the patch and the tests, but if PS_UPDATE_FUNC() doesn't exist, how would you call it? The session ID is something consumed by (or passed to) the session handler, there's a big difference. > "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. Again, nothing explains how that compatibility is maintaned and this is exactly the concern that I have. When something is unknown, it usually produces side effects. Cheers, Andrey.