# [VOTE] Introduce session.lock, session.lazy_write and session.lazy_destory - Thread: https://externals.io/message/71287 - Original archive: https://news-web.php.net/php.internals/71287 - Started by Yasuo Ohgaki on 2014-01-20T00:37:51+00:00 - Messages: 42 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/71287.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #71287 — Yasuo Ohgaki — 2014-01-20T00:37:51+00:00 URL: https://externals.io/message/71287#71287 Hi all, New session feature has been discussed while. Vote period: 2014/01/20 - 2014/01/30 https://wiki.php.net/rfc/session-lock-ini New comments and questions are welcomed. Thank you for voting. P.S. I have removed session_discard(). I've committed it as `session_abort()` to address FR last year. Name/alias is open to be changed. Please comment on different thread for this. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71290 — Stas Malyshev — 2014-01-20T02:56:31+00:00 URL: https://externals.io/message/71287#71290 · In reply to: #71287 Hi! > Vote period: 2014/01/20 - 2014/01/30 > https://wiki.php.net/rfc/session-lock-ini > > New comments and questions are welcomed. I see there are three proposals there, but only one vote. E.g. I think unlocked sessions are not a good idea, but lazy write may be ok (btw, interaction between no-lock and lazy write can lead to completely undebuggable scenarios when same data by same code sometimes are written and sometimes are not depending on session state). I'm not sure I even understand what lazy_destroy does - if the data is deleted then it's deleted, how can it be accessible and why? Also, do I understand correctly that the options would apply only to files module? If so, why they under session.* and not session.files.* or something to emphasize that if you're not using files module it doesn't do anything? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #71293 — Yasuo Ohgaki — 2014-01-20T04:27:03+00:00 URL: https://externals.io/message/71287#71293 · In reply to: #71290 Hi Stas, On Mon, Jan 20, 2014 at 11:56 AM, Stas Malyshev wrote: > > […] > > I see there are three proposals there, but only one vote. E.g. I think > unlocked sessions are not a good idea, but lazy write may be ok (btw, > interaction between no-lock and lazy write can lead to completely > undebuggable scenarios when same data by same code sometimes are written > and sometimes are not depending on session state). It is made a single RFC so that everyone understand new features. I agree that unlocked session could be a cause of hard to debug bugs and it should never be a default. Even if unlocked session can be dangerous, there are save handlers that support unlocked session for better performance. e.g. memcache. It would be reasonable to support by module. Unlocked session could be more usable by session.lazy_write. According to current implementation, "the last session wins". With session.lazy_write, "the last session *writes data* wins". This behavior prevents loosing updated data. It's better for performance also. > I'm not sure I even > understand what lazy_destroy does - if the data is deleted then it's > deleted, how can it be accessible and why? > With AJAX or browser supports concurrent access to server, concurrent access to server is possible. When session ID is regenerated, it is possible that some connections access to server with old session ID. i.e. Race condition. When this happened, old session that may be known to attacker may be reinitialized or new unneeded session ID is created when use_strict_mode=On. Allowing access to old session data for a while prevents these cases that initialize unneeded session. RFC proposes deletion flag in $_SESSION. It's a little dirty, but it's faster and simpler. It would be acceptable because the session is deleted one. Also, do I understand correctly that the options would apply only to > files module? If so, why they under session.* and not session.files.* or > something to emphasize that if you're not using files module it doesn't > do anything? No. It's applicable to all save handlers. files save handler is default save handler and there were comments that want separate implementation for files. That's the reason why there is such voting option. files and files_ext would be identical code except the code supports proposed options. I would like to have single code for easier maintenace. IMHO. Thank you for the comment. I hope everyone understands idea behind this RFC. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71294 — Yasuo Ohgaki — 2014-01-20T04:45:54+00:00 URL: https://externals.io/message/71287#71294 · In reply to: #71293 Hi all, > With AJAX or browser supports concurrent access to server, > concurrent access to server is possible. > > When session ID is regenerated, it is possible that some connections > access to > server with old session ID. i.e. Race condition. > > When this happened, old session that may be known to attacker may be > reinitialized or new unneeded session ID is created when > use_strict_mode=On. > > Allowing access to old session data for a while prevents these cases that > initialize > unneeded session. > > RFC proposes deletion flag in $_SESSION. It's a little dirty, > but it's faster and simpler. It would be acceptable because the session is > deleted one. > Additional comment for this. `session_destroy()`/session_gc() deletes session data immediately. session.lazy_destroy is applicable only when `session_regenerate_id()` is called. The name (session.lazy_destroy) might be better to be changed or it might be better apply for `session_destroy()` since there would be similar race condition like `session_regenerate_id()`. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71295 — Stas Malyshev — 2014-01-20T05:04:53+00:00 URL: https://externals.io/message/71287#71295 · In reply to: #71293 Hi! > Unlocked session could be more usable by session.lazy_write. According > to current implementation, "the last session wins". With session.lazy_write, "the > last session *writes data* wins". This behavior prevents loosing updated data. It's > better for performance also. I think it's not a good idea. This creates a setup where you never know if your action on session state would actually result in any action at all. It would be as good for performance as a database that skips random data writes. Performance may be great, but nobody would use such database because it's unreliable. I don't think we should put into PHP a mechanism that sets up users for trouble. > With AJAX or browser supports concurrent access to server, > concurrent access to server is possible. > > When session ID is regenerated, it is possible that some connections > access to > server with old session ID. i.e. Race condition. I don't see how it is possible. I.e. it can access with the old ID, but if it happened before the session was deleted, then it gets the session, if it happens after, it gets "no session". Isn't that why you proposed strict sessions? It of course is also possible to handle without it - request after deletion would get empty session state and would have to return error message and have the client to re-request with correct session state. Do you mean that somebody would regenerate the id but not destroy the old session? I don't see how it would be useful - if you have any security context, the whole point is to destroy the old session, why won't you do that? > When this happened, old session that may be known to attacker may be > reinitialized or new unneeded session ID is created when use_strict_mode=On. It of course may be reinitialized, but what's wrong with that? Deleted data would not be accessible anyway - the data is already gone. > Allowing access to old session data for a while prevents these cases > that initialize unneeded session. *That* would be a security hole - the system thinks the user was logged out but you allow anybody who still has old session ID still access the session as if nothing happened? That's not good at all. > No. It's applicable to all save handlers. Then what's the point of proposing additional handler if it applies to all handlers anyway? > files save handler is default save handler and there were comments that > want separate implementation for files. That's the reason why there is such > voting option. > > files and files_ext would be identical code except the code supports > proposed options. I would like to have single code for easier > maintenace. IMHO. I don't understand. If all save handlers have this option, what is the point of existence of both files and files_ext? And what about other handlers - do they also have to have _ext versions? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #71299 — Yasuo Ohgaki — 2014-01-20T07:13:53+00:00 URL: https://externals.io/message/71287#71299 · In reply to: #71295 Hi Stas, On Mon, Jan 20, 2014 at 2:04 PM, Stas Malyshev wrote: > > […] > session.lazy_write, "the > > […] > data. It's > > […] > > I think it's not a good idea. This creates a setup where you never know > if your action on session state would actually result in any action at > all. It would be as good for performance as a database that skips random > data writes. Performance may be great, but nobody would use such > database because it's unreliable. I don't think we should put into PHP a > mechanism that sets up users for trouble. I agree that users could get into trouble if they don't know what they are doing. To get most out of session.lock=off and session.lazy_write=on, proper usage is needed to get reliable operation Suppose session only stores authentication info, then session and data storage only needs read. Session write (or destroy) is only needed when users are logged out. Session write is not needed almost all. PHP could cut session and storage load to half and PHP could allow *concurrent* access for apps while app could maintain consistency perfectly. In this scenario, app performance could be boosted a lot by removed write() calls. Programmer also don't have to care to call `session_commit()`/session_write_close() to get concurrency. Session module writes session data when it is needed. (Programmers must be careful about concurrent write() calls, though) At least I'll use this and there would be certain number of users who use like this for better performance. Users, who want correct access counts or like, should not use session.lock=off. session.lazy_write=on will not give any benefits if program updates $_SESSION for every request. > With AJAX or browser supports concurrent access to server, > > […] > > I don't see how it is possible. I.e. it can access with the old ID, but > if it happened before the session was deleted, then it gets the session, > if it happens after, it gets "no session". Isn't that why you proposed > strict sessions? Strict session is to prevent session to initialize with user supplied session ID. In this case, session ID is initialized by PHP and I would like to allow access to deleted session for a while to prevent race condition that creates unneeded new session ID (and it's data). > It of course is also possible to handle without it - > request after deletion would get empty session state and would have to > return error message and have the client to re-request with correct > session state. > Current session module implementation and web technology allows to access old session ID when session ID is regenerated. As you described, session X could remain deleted. Request A Request B ↓ | Access session X ↓ | Access session X (if locked, Req. B waits ) ↓ ↓ Regenerate ID (Del X, New Y) session X unlocked (Del X unlocks) ↓ ↓ Close session Y Close session X (For files, inode is marked deleted remain deleted) However, this could happen also. Request A Request B ↓ | Access session X | ↓ | Regenerate ID (Del X, New Y) | ↓ | Close session Y | Access session X ↓ Session X is created (use_strict_mode=on creates session Z) ↓ Close session X (Session X or Z remains) Without strict session, old session X could remain. With strict session, unneeded Z could remain. Even if programmers want to delete session X, session X could be reinitialized by default config. There are more cases with session.lock=off. > Do you mean that somebody would regenerate the id but not destroy the > old session? I don't see how it would be useful - if you have any > security context, the whole point is to destroy the old session, why > won't you do that? > It's described in previous. Users wouldn't, but system might do. > > […] > use_strict_mode=On. > > It of course may be reinitialized, but what's wrong with that? Deleted > data would not be accessible anyway - the data is already gone. > I have to double check, but for example, current files do not wipe data file content. It's a simple unlink. I suppose it has old data. It will be inaccessible eventually as described, though. For database based session, reading deleted data should be result in error. (e.g. Serializable transaction isolation level is needed for this) > > […] > > *That* would be a security hole - the system thinks the user was logged > out but you allow anybody who still has old session ID still access the > session as if nothing happened? That's not good at all. It allows access to old session for certain amount of time. 30 seconds would be enough for session.lock=off. 60 or more when session.lock=on. I was about to allow access to deleted session for 90 seconds. Configurable INI may be better. I'll make session.lazy_destroy a integer config. > > […] > > Then what's the point of proposing additional handler if it applies to > all handlers anyway? > files may simply ignore new INI config since there may not be codes support new options. files_ext is "files + new INI config support". IIRC you're one of them who are preferred to have separate implementation(?) I don't mind at all to remove files_ext proposal. > > […] > such > > […] > > I don't understand. If all save handlers have this option, what is the > point of existence of both files and files_ext? And what about other > handlers - do they also have to have _ext versions? No, they don't have to. They can ignore new features or implement them all or part of them. I would like to know session save handler capability. I'll add save handler API that declares it's capability. I'll update the RFC later. Thank you for your comment. The RFC is getting better. I understand your anxiety. I'm not going to change current behavior, just adding new features for advanced save handlers and users. I'll write big caution to the manual also. Shooting their own foot is possible, but let advanced users to enjoy extreme performance! Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71301 — Stas Malyshev — 2014-01-20T07:35:33+00:00 URL: https://externals.io/message/71287#71301 · In reply to: #71299 Hi! > It allows access to old session for certain amount of time. 30 seconds > would be enough for session.lock=off. 60 or more when session.lock=on. > I was about to allow access to deleted session for 90 seconds. > Configurable INI may be better. I'll make session.lazy_destroy a integer > config. It is irrelevant how long it is. Even 1 microsecond is long enough - if the app said session data should be gone, then they should be gone, not linger around and allow access to them. If you need access to that data, copy it into the new session. > IIRC you're one of them who are preferred to have separate > implementation(?) I prefer not to have implementation for 2 out of 3 proposed "features" at all, because I think they will produce more problems than solutions. For the lazy-write feature, IMO it can be made global, or function (I actually prefer both) - but I see no reason to create a duplicate driver, if you don't want it turned on, turn off the option. I'm not sure what would be the reason to clone the driver. Do you have a reference for where it was asked so I could re-read it and recall what were the reasons for it? My memory may be failing me :) > No, they don't have to. They can ignore new features or implement them > all or part of them. This would be a bit confusing - you set an option, but some drivers would implement it, some would ignore it, so you never know what actually happens. > Shooting their own foot is possible, but let advanced users to enjoy > extreme performance! I don't think performance at he expense of reliability, data consistency and security should be our goal. Like I said, we could also make an option to ignore random number of file writes and claim it is a performance feature, but it would be useless, since there is no case where one would recommend to use it. I see no case where I would recommend anybody to use unlocked sessions - so why introduce it? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #71306 — Yasuo Ohgaki — 2014-01-20T08:22:52+00:00 URL: https://externals.io/message/71287#71306 · In reply to: #71301 Hi Stas On Mon, Jan 20, 2014 at 4:35 PM, Stas Malyshev wrote: > > […] > > It is irrelevant how long it is. Even 1 microsecond is long enough - if > the app said session data should be gone, then they should be gone, not > linger around and allow access to them. If you need access to that data, > copy it into the new session. > I agree. If app delete it now, it should be delete immediately. It is possible with session.lazy_destory=0. Since server and client is not synchronized, there are edge case that app may allow to access old session for a while. It should be allowed. IMO. > > > […] > > I prefer not to have implementation for 2 out of 3 proposed "features" > at all, because I think they will produce more problems than solutions. > For the lazy-write feature, IMO it can be made global, or function (I > actually prefer both) - but I see no reason to create a duplicate > driver, if you don't want it turned on, turn off the option. I'm not > sure what would be the reason to clone the driver. Do you have a > reference for where it was asked so I could re-read it and recall what > were the reasons for it? My memory may be failing me :) > > […] > > This would be a bit confusing - you set an option, but some drivers > would implement it, some would ignore it, so you never know what > actually happens. > That's the reason why I would like to add new API to declare save handler capability. If I know capability, I may assume what it does. > > […] > > I don't think performance at he expense of reliability, data consistency > and security should be our goal. Like I said, we could also make an > option to ignore random number of file writes and claim it is a > performance feature, but it would be useless, since there is no case > where one would recommend to use it. I see no case where I would > recommend anybody to use unlocked sessions - so why introduce it? In previous mail, it is described good use case. Without locking, scripts are executed concurrently. When session.lock=off and session.lazy_write=on, scripts does not have to wait session read() and only updated session is written back to storage. For example, if session only store authentication information, it works perfectly and there is no risk of authenticated(logged out session) remains or un-authenticated session became an authenticated session. We also should consider that memcache/memched have unlock option already. Like databases allow query without transaction or transaction with less consistency, session module may allow options that give more performance at the cost of consistency. It's an option for developers. IMHO. (Current session is the same as "serializable transaction isolation level" only database which is very slow and restrict concurrency.) I'll write clearly that "end users" should never enable dangerous options unless developer explicitly allows it. I'll also write that developers are better to detect dangerous options if app does not support them. Is it enough, isn't it? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71307 — Bas van Beek — 2014-01-20T08:27:07+00:00 URL: https://externals.io/message/71287#71307 · In reply to: #71287 Hi, op 20-01-14 01:37, Yasuo Ohgaki schreef: > Hi all, > > New session feature has been discussed while. > > Vote period: 2014/01/20 - 2014/01/30 > https://wiki.php.net/rfc/session-lock-ini > > New comments and questions are welcomed. It's funny you mention the memcache session save handler. I remember when it was introduced quite some people were scratching their heads about unexpected session data behavior. Nothing in the module documentation even talked about it not locking the session data by default, therefore last script exiting writing it's version of the session data (where a script not changing the session data would write back the old session state removing other scripts' changes as it was just last to end. so much fun debugging non consistent session changes as a result of scripts ending in a different order). I thought it was really bad behavior and complained about it then. The things you try to solve (concurrent access / speeding up performance) can for the most part already be done with the current session logic in a well defined consistent manner. opening and locking the session, reading and writing the needed session variables, closing and unlocking the session with session_write_close and continuing script execution after which other scripts have access to the session. For many programmers a familiar pattern similar to critical sections and mutexes in threaded environments. I see only one very simple function potentially being helpful. A simple session close without write maybe called session_close(). This would give a performance bonus as it will not need to serialize the session data and write it back to session storage. It also helps to programmer in cases that he wants to guarantee that the session store is not changed by script (similar to a transaction abort). just my $.02 Bas van Beek > Thank you for voting. > > P.S. I have removed session_discard(). I've committed > it as `session_abort()` to address FR last year. Name/alias > is open to be changed. Please comment on different thread > for this. > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #71308 — Yasuo Ohgaki — 2014-01-20T08:36:29+00:00 URL: https://externals.io/message/71287#71308 · In reply to: #71307 Hi Bas, > op 20-01-14 01:37, Yasuo Ohgaki schreef: > > Hi all, > > […] > > It's funny you mention the memcache session save handler. > Are they removed or I remembered incorrectly. This is taken from memcached 2.2.0b1 php_memcached.c: STD_PHP_INI_ENTRY("memcached.sess_locking", "1", PHP_INI_ALL, OnUpdateBool, sess_locking_enabled, zend_php_memcached_globals, php_memcached_globals) I should update RFC. Thank you. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71309 — Yasuo Ohgaki — 2014-01-20T08:42:38+00:00 URL: https://externals.io/message/71287#71309 · In reply to: #71307 Hi Bas, > I see only one very simple function potentially being helpful. A simple > session close without write maybe called session_close(). This would give > a performance bonus as it will not need to serialize the session data and > write it back to session storage. It also helps to programmer in cases > that he wants to guarantee that the session store is not changed by > script (similar to a transaction abort). > I have already added this as `session_abort()`. Since it's only in 5.6 and master branch, the name could be changed. However, `session_abort()` (or something like session_discard()) would be better name because the name implies it does not save data. If you come up with better name, please let me know. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71311 — Stas Malyshev — 2014-01-20T08:50:30+00:00 URL: https://externals.io/message/71287#71311 · In reply to: #71306 Hi! > In previous mail, it is described good use case. > Without locking, scripts are executed concurrently. If the scripts are going to write the session, they can not execute concurrently, because that creates race condition and at least one of the states will be lost. If they don't intend to write, they can just drop the lock immediately after reading, that's why we talked about having function for that. Neither works well with no locks at all. > When session.lock=off and session.lazy_write=on, scripts does not have > to wait session read() and only updated session is written back to storage. > For example, if session only store authentication information, it works > perfectly and there is no risk of authenticated(logged out session) remains It doesn't work perfectly for scripts that modify authentication information. And writing an app where there are race conditions in authentication mechanism is very poor idea, security-wise. For performance in read-only case, there's a simple solution which does not jeopardize stability, see above. > We also should consider that memcache/memched have unlock option already. I would not recommend anybody then to use them with that option. This only can lead to trouble. It's running shared data structure without any locks. If it's not specially designed for it, you'll get in trouble - and it will always be undebuggable, unreproducible and happen in the worst possible moment, when the system is loaded and too many things are happening at once. > Like databases allow query without transaction or transaction with less If you are using DB without transactions, you must ensure consistent state in your app, or accept data inconsistency. Data inconsistency may be ok if your data is not that important - but if it's security-sensitive, it is not a good idea. Sessions usually deal with security-sensitive data. > I'll write clearly that "end users" should never enable dangerous > options unless > developer explicitly allows it. I'll also write that developers are I don't think we should provide options that need disclaimers that say "do not use it unless in very rare cases". If you are experienced enough that you need this rare case, you can implement your own session handler that does that. Especially given that the only use case I have seen so far actually does not need this feature and works just fine with much safer and smaller feature (session unlock, aka session_discard, aka session_abort). -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #71313 — Yasuo Ohgaki — 2014-01-20T09:17:10+00:00 URL: https://externals.io/message/71287#71313 · In reply to: #71311 Hi Stas, On Mon, Jan 20, 2014 at 5:50 PM, Stas Malyshev wrote: > > […] > > I don't think we should provide options that need disclaimers that say > "do not use it unless in very rare cases". If you are experienced enough > that you need this rare case, you can implement your own session handler > that does that. Especially given that the only use case I have seen so > far actually does not need this feature and works just fine with much > safer and smaller feature (session unlock, aka session_discard, aka > session_abort). This is feasible option. There is `session_abort()` already. It's nice to have session_unlock(). I'll add session_unlock(). It requires new save handler API, but API is modified anyway. I understand your discussion and anxiety. People do stupid things w/o knowing what they are doing. Yet, I would like to have "non transaction query" like session data handling, since there are apps that work as it should without additional/modified code and enjoy much better performance. I'll change voting option so that people could vote feature by feature. There are 3 users including me who vote to this RFC. They have to vote again. I'll send mail when I finish editing the RFC, please hold to vote. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71319 — Yasuo Ohgaki — 2014-01-20T10:30:57+00:00 URL: https://externals.io/message/71287#71319 · In reply to: #71313 Hi all, > I'll send mail when I finish editing the RFC, please hold to vote. Before I re-open vote, I would like to ensure if the RFC has no issues to discuss. https://wiki.php.net/rfc/session-lock-ini Thank you for reviewing RFC! Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71322 — Yasuo Ohgaki — 2014-01-20T12:07:07+00:00 URL: https://externals.io/message/71287#71322 · In reply to: #71319 Hi Stas, > > > > […] > > > Before I re-open vote, I would like to ensure if the RFC has no > issues to discuss. > > https://wiki.php.net/rfc/session-lock-ini > > Thank you for reviewing RFC! I was forgetting whole point of having session.lazy_destroy. This feature should be enabled by default to destroy old session when `session_regenerate_id()` is called. Regenerating session ID with reliable manner is mandatory for better security. Therefore, I've changed RFC to enable session.lazy_destroy by default. Default delay is matter to discuss, though. Please refer to the referenced discussion in the RFC. http://marc.info/?l=php-internals&m=138242492914526&w=2 Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71324 — johannes@schlueters.de — 2014-01-20T14:18:06+00:00 URL: https://externals.io/message/71287#71324 · In reply to: #71309 > Hi Bas, > > > > > […] > > I have already added this as `session_abort()`. > Since it's only in 5.6 and master branch, the name could be changed. > However, `session_abort()` (or something like session_discard()) would be > better name because the name implies it does not save data. > > If you come up with better name, please let me know. I think those two are useful: - `session_abort()` - close session, do *not* write, unlock file - session_close() - close session, *do* write, unlock file I do think this also should be application defined, not configuration defined. So using functions for that is better than ini options. Only the individual script nows what it needs, global configuration for tuning one application will break another on the same system and makes it harder to write portable appliations. johannes --- ## #71369 — Yasuo Ohgaki — 2014-01-20T23:06:25+00:00 URL: https://externals.io/message/71287#71369 · In reply to: #71324 Hi Johanns and Stas, - session_close() - close session, *do* write, unlock file We have `session_write_close()` (session_commit() is alias), so it may be better to remain as it is. On Mon, Jan 20, 2014 at 11:18 PM, Johannes Schlüter wrote: > I do think this also should be application defined, not configuration > defined. So using functions for that is better than ini options. Only > the individual script nows what it needs, global configuration for > tuning one application will break another on the same system and makes > it harder to write portable appliations. > Stas and your discussion sounds reasonable to me, too. I don't mind files_ext implementation to be a PECL module as a reference implementation. It would be a module that does not provide any function, but only a files_ext save handler and lock INI. session_unlock() is a simple new function and it does not require RFC, but it is mentioned in it. However, unlocking session after start makes thing more complex for save handlers. It is better to specify when session is started. Therefore, it would be better to pass new behavior options to `session_start()`. session_start(array('lock'=>false, 'lazy_write'=>true, 'lazy_destroy'=>120)); It may also set other options for session like save handler, serializer, expire, etc. Since session.lazy_destroy is required for reliable `session_regenerate_id()` operation, it remains but not exposed as INI option. Users has to pass option to `session_start()` to disable it. In short, no new INI is added. Option parameter is added to `session_start()`. I'll add new API for save handler to get start options. I would like to change the RFC like this. Any comments? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71372 — Yasuo Ohgaki — 2014-01-21T01:08:25+00:00 URL: https://externals.io/message/71287#71372 · In reply to: #71369 Hi all, > I would like to change the RFC like this. > Any comments? > I've updated the RFC. https://wiki.php.net/rfc/session-lock-ini Removed INI options and options are `session_start()` parameter now. It has changed to satisfy most comments hopefully. Could you review the RFC before reopen vote? Thank you. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71765 — Yasuo Ohgaki — 2014-01-30T00:29:50+00:00 URL: https://externals.io/message/71287#71765 · In reply to: #71287 Hi all, https://wiki.php.net/rfc/session-lock-ini Before I re-open vote of this RFC, I would like to add 'read_only' parameter for just reading session and close immediately. This was suggested by Bill Salak. This option is useful and the fastest way to read session. I updated the RFC. There are 6 vote options now. It would be too much. Does anyone mind if I make the RFC a single vote? Or should I separate Proposal 2 (minimize_lock)? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71766 — Stas Malyshev — 2014-01-30T01:09:05+00:00 URL: https://externals.io/message/71287#71766 · In reply to: #71765 Hi! > There are 6 vote options now. It would be too much. > Does anyone mind if I make the RFC a single vote? > Or should I separate Proposal 2 (minimize_lock)? 6 options seems too much. 0 shouln't even be a vote - if people want specific options, then ipso facto they want them introduced, and parameter is a way to do it, if none is accepted - there's no reason to introduce the parameter. I would rather make it part of the whole proposal and propose both INI value and session_start options (with the latter overriding the former). Proposal 5 seems to have nothing to do with the rest. So it probably needs to be split out. I'd just leave it as two proposals (this is my personal preference): 1. Options & read_only + lazy_write (these ones seem to be least controversial ones and don't seem to have any negative implications) 2. lazy_destroy 3. minimize_lock if you really want it though I still think it is dangerous and is not needed if read_only and lazy-write exist and if you still need it you're doing sessions wrong. But if you really think there's a case then let's put it as separate one. So we'd have 2 or 3 options, which I think is the max for vote to be manageable. Some more things: - session_unlock - I don't see how it's different from session_abort. What's the need for it? - session_module_feature - what is the use case for it? I'm not sure I understand why anybody would use it. Especially if outputs would be non-machine-friendly as "Partially Supported" - what would you do with that? What Partially means here? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #71768 — Yasuo Ohgaki — 2014-01-30T03:01:39+00:00 URL: https://externals.io/message/71287#71768 · In reply to: #71766 Hi Stas, On Thu, Jan 30, 2014 at 10:09 AM, Stas Malyshev wrote: > 6 options seems too much. 0 shouln't even be a vote - if people want > specific options, then ipso facto they want them introduced, and > parameter is a way to do it, if none is accepted - there's no reason to > introduce the parameter. I would rather make it part of the whole > proposal and propose both INI value and session_start options (with the > latter overriding the former). > > Proposal 5 seems to have nothing to do with the rest. So it probably > needs to be split out. > > I'd just leave it as two proposals (this is my personal preference): > > 1. Options & read_only + lazy_write (these ones seem to be least > controversial ones and don't seem to have any negative implications) > 2. lazy_destroy > 3. minimize_lock if you really want it though I still think it is > dangerous and is not needed if read_only and lazy-write exist and if you > still need it you're doing sessions wrong. But if you really think > there's a case then let's put it as separate one. > > So we'd have 2 or 3 options, which I think is the max for vote to be > manageable. > Sounds reasonable. I'll update RFC according to your comment. minimize_lock is still useful for users would like to write only when $_SESSION is updated. Without it, session data is locked and scripts are not executed in parallel. Programmer has to selectively start session read only and re-start session for writing when it's needed. Efficient session management could be done as I wrote/you think, so it's not strictly required. 'minimize_lock' make things easier (no selective read only session nor re-starting session for writing) for experts when consistency is under control. Some more things: > - session_unlock - I don't see how it's different from session_abort. > What's the need for it? > I should have change this description. `session_abort()` closes session without writing data. session_unlock() unlock session data. Data is written when `session_commit()`/etc. session_unlock() is not needed if there is 'minimize_lock'. Unlike 'minimize_lock', it will slow down performance. It will not be implemented. > - session_module_feature - what is the use case for it? I'm not sure I > understand why anybody would use it. Especially if outputs would be > non-machine-friendly as "Partially Supported" - what would you do with > that? What Partially means here? > This is informational function for programmers, not for programs. It requires active session to be useful because it needs active save handler module. It's rather suitable as `phpinfo()`, but it might not be useful for users. Since is required to be useful. It may return result as constants. I'm not sure if it is useful. If anyone insists, I'll change it return constants. Meaning of "Partially Supported" depends on save handler. It should be documented in save handler manual what does it mean. For example, `session_gc()` with file handler works for single directory, but it's not for multiple directory. We had number of not-a-bug reports for this. I would like to reopen vote next week. Anyone who has comments, please send them now. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71770 — Patrick Schaaf — 2014-01-30T04:41:41+00:00 URL: https://externals.io/message/71287#71770 · In reply to: #71768 Hello Yasuo, First of all, the github "compare" link you give in the RFC shows a vast amount of unrelated changed files. Playing around a bit I find that comparing not to php:master but to yohgaki:PHP-5.6 gives a better overview: https://github.com/yohgaki/php-src/compare/PHP-5.6...PHP-5.6-rfc-session-lock Regarding minimize_lock: Generally a really dangerous feature. minimize_lock sounds so friendly, does not imply danger really. Alternative naming proposal: unlocked_thus_unsafe The minimize_lock=true implementation in mod_files.c looks wrong: 1) in PS_READ_FUNC the flock(LOCK_EX) is placed after `fstat()`. A concurrent writer could rewrite the file between the fstat and the pread/read, resulting in either a short read or an incomplete read, in both cases resulting in invalid session data being read. 2) in PS_WRITE_FUNC the flock(LOCK_EX) is placed after `ftruncate()`. Thus, an ftruncate could hit both concurrent readers and writers at any time, resulting in various kinds of problems. 3) again in PS_WRITE_FUNC in the check whether to ftruncate, data->st_size, which was set in an earlier PS_READ_FUNC, is used for the comparison. However, the file on disk may have been changed by a concurrent writer in the meantime, and have an arbitrary different size, resulting in a wrong decision to ftruncate-or-not. To solve all three problems, I'd suggest - unconditional taking of the flock(LOCK_EX) in ps_files_open(), while keeping the `LOCK_UN` in PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true). - also when minimize_lock=true, make an `fstat()` call in PS_WRITE_FUNC before the need-to-ftruncate check, ignoring data->st_size from the read. Reading that code, also in the base PHP repository, I think I spotted an unrelated bug: In ps_files_open() in the not-WIN32-open_basedir fstat/S_ISLNK _error_ cases: data->fd is closed, but data->fd is then NOT set to -1. best regards Patrick --- ## #71771 — Patrick Schaaf — 2014-01-30T04:50:40+00:00 URL: https://externals.io/message/71287#71771 · In reply to: #71770 > To solve all three problems, I'd suggest > > - unconditional taking of the flock(LOCK_EX) in ps_files_open(), while keeping > the `LOCK_UN` in PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true). Clarification: unconditionally in case of minimize_lock=true means taking the `LOCK_EX` after the huge if, right at the end of ps_files_open, i.e. even when data->fd was already >= 0. Otherwise the write case would miss the lock. If that seems strange in ps_files_open then the `LOCK_EX` could be placed in PS_READ_FUNC and PS_WRITE_FUNC directly after successful call to ps_files_open - doing it that way would have the advantage of making a local code review in each of the functions self-contained wrt the locking. best regards Patrick --- ## #71772 — Yasuo Ohgaki — 2014-01-30T05:34:35+00:00 URL: https://externals.io/message/71287#71772 · In reply to: #71770 Hi Patrick, > First of all, the github "compare" link you give in the RFC shows a vast > amount of unrelated changed files. Playing around a bit I find that > comparing > not to php:master but to yohgaki:PHP-5.6 gives a better overview: > > > https://github.com/yohgaki/php-src/compare/PHP-5.6...PHP-5.6-rfc-session-lock > > Regarding minimize_lock: > > Generally a really dangerous feature. minimize_lock sounds so friendly, > does > not imply danger really. > > Alternative naming proposal: unlocked_thus_unsafe > It sounds good idea. How about shorter name? unsafe_lock > To solve all three problems, I'd suggest > > - unconditional taking of the flock(LOCK_EX) in ps_files_open(), while > keeping > the `LOCK_UN` in PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true). > This would be safer and good for reference implementation. > - also when minimize_lock=true, make an `fstat()` call in PS_WRITE_FUNC > before > the need-to-ftruncate check, ignoring data->st_size from the read. > I'll fix this later in my branch. > Reading that code, also in the base PHP repository, I think I spotted an > unrelated bug: In ps_files_open() in the not-WIN32-open_basedir > fstat/S_ISLNK > _error_ cases: data->fd is closed, but data->fd is then NOT set to -1. > This should be set to -1. I've committed the fix. Thank you. Regards. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #71773 — Patrick Schaaf — 2014-01-30T05:44:41+00:00 URL: https://externals.io/message/71287#71773 · In reply to: #71772 On Thursday 30 January 2014 14:34:35 Yasuo Ohgaki wrote: > > > […] > > It sounds good idea. How about shorter name? > > unsafe_lock Well, from the user (PHP code) level it is not a lock at all. The localized flocking is purely for internal consistency of the individual read or write operation, in that case. That's why I wanted to have 'unlocked' in the name. And for such a dangerous option (*) I think that a very expressive long name would be a good fit. best regards Patrick (*) yes I remember that I suggested it in the first place, back in October :) --- Next page: https://externals.io/message/71287.md?page=2