# Revert session_serializer_name(), session_gc() - Thread: https://externals.io/message/73066 - Original archive: https://news-web.php.net/php.internals/73066 - Started by Andrey Andreev on 2014-03-11T13:37:44+00:00 - Messages: 41 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/73066.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 --- ## #73066 — Andrey Andreev — 2014-03-11T13:37:44+00:00 URL: https://externals.io/message/73066#73066 Hello, I just saw this in the 5.6-alpha1 changelog: - Implemented Request #54649 (Create session_serializer_name()). (Yasuo) ... - Implemented Request #11100 (session_gc() function). (Yasuo) I didn't find an RFC and I don't know a way to search for a discussion on internals (Google doesn't find one). session_serializer_name(): https://bugs.php.net/bug.php?id=54649 I don't think that this should've been implemented, especially with the reasoning from the feature request ... The same could be applied to all session ini options and especially now since `session_start()` accepts them, it's useless IMO. `session_gc()` was added in a similar fashion: https://bugs.php.net/bug.php?id=11100 There's already a comment about how the same thing can be achieved by overloading SessionHandler::gc(), but a user could also just alter session.gc_divisor, session.gc_probability to ensure that the garbage collector is started. And while session_serializer_name() is just redundant, `session_gc()` could cause performance issues. I'd strongly suggest that these 2 functions should be removed before PHP 5.6 is officially released. Regards, Andrey Andreev. --- ## #73069 — Yasuo Ohgaki — 2014-03-11T22:37:31+00:00 URL: https://externals.io/message/73066#73069 · In reply to: #73066 Hi Andrey, > I just saw this in the 5.6-alpha1 changelog: > > - Implemented Request #54649 (Create session_serializer_name()). (Yasuo) > ... > - Implemented Request #11100 (session_gc() function). (Yasuo) > > > I didn't find an RFC and I don't know a way to search for a discussion > on internals (Google doesn't find one). > > session_serializer_name(): https://bugs.php.net/bug.php?id=54649 > > I don't think that this should've been implemented, especially with > the reasoning from the feature request ... The same could be applied > to all session ini options and especially now since `session_start()` > accepts them, it's useless IMO. > > `session_gc()` was added in a similar fashion: > https://bugs.php.net/bug.php?id=11100 > > There's already a comment about how the same thing can be achieved by > overloading SessionHandler::gc(), but a user could also just alter > session.gc_divisor, session.gc_probability to ensure that the garbage > collector is started. > Users want to have contorl *when* gc is performed rather than luck. This is reasonable. IMO. > And while session_serializer_name() is just redundant, `session_gc()` > could cause performance issues. > There is `session_module_name()`, why not session_serializer_name()? Many 3rd party save handler implements many serializer. `session_gc()` is nothing to do with performance. Users may abuse. The same argument applies to gc probability. I'd strongly suggest that these 2 functions should be removed before > PHP 5.6 is officially released. > These are simple additional APIs. If your argument applies, there should be number of new RFCs. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73070 — Andrey Andreev — 2014-03-11T23:54:21+00:00 URL: https://externals.io/message/73066#73070 · In reply to: #73069 Hi Yasuo, > Hi Andrey, > > > > […] > > > Users want to have contorl *when* gc is performed rather than luck. This is > reasonable. IMO. I like control, it's nice and *I already have it*: ini_set('session.gc_probability', 1); ini_set('session.gc_divisor', 1); > > […] > > There is `session_module_name()`, why not session_serializer_name()? Why not session_cookie_lifetime()? Why not `session_cache_limiter()`? Why not session_entropy_file()? etc. "Why not" is never a good argument in programming and really, when is the last time that you've seen something accepted here on internals because of "why not"? I'd rather question if `session_module_name()` is useful, or any function that's an alias for ini_set('something', ...). > Many 3rd party save handler implements many serializer. I haven't seen many. In fact, I've seen none. > `session_gc()` is nothing to do with performance. Users may abuse. The same > argument applies to gc probability. The problem is, users WILL abuse and that's bad. Think of what the manual page would say about that function. Something in the lines of: Calls the session garbage collector, ignoring session.gc_probablity and session.gc_divisor. What does that say to an inexperienced and even to the intermediate user, who doesn't even know what GC is? "Hey, this collects garbage! It's nice to clean things up, I should always call it!" Yet another bad practice spreads. Sure, that *could* apply to gc_probability, gc_divisor, but those are *necessary* options and they are not easy to find unless you are specifically looking for them. > > > […] > > > These are simple additional APIs. > If your argument applies, there should be number of new RFCs. Actually, if my argument applies - both feature requests should've been ignored. Lots of people make "this would be nice" feature requests without having the knowledge and experience to know if it would really be "nice". That's why there's a bunch of smart people here to take the decisions for them, collectively. I myself have been forced to write an RFC for a smaller and less impactful change, but that's not the point. Both of these functions should've at least been discussed here on internals. And finally, do you assume that there's demand for these features? `session_gc()` specifically was requested 13 YEARS ago and it didn't even get a "+1" comment in that timeframe. There are people on this list who'd question user demand even if people are tweeting about the feature with fingers crossed that it would be implemented ... these are two that hardly anybody cares about. Cheers, Andrey. --- ## #73071 — Yasuo Ohgaki — 2014-03-12T00:19:14+00:00 URL: https://externals.io/message/73066#73071 · In reply to: #73070 Hi Andrey, > > […] > is > > […] > > I like control, it's nice and *I already have it*: > > ini_set('session.gc_probability', 1); > ini_set('session.gc_divisor', 1); Of course you can. It does not look nice as API. API should have good name for it. IMO. `session_gc()` is useful for both low traffic site and high traffic site. I don't see reason not to have explicit gc API. User may simply call `session_gc()`; periodically by cron, etc. > > […] > > Why not session_cookie_lifetime()? > Why not `session_cache_limiter()`? > Why not session_entropy_file()? > etc. Those aren't changed by programs often. As I wrote, 3rd party save handler implements their own serializers. Not like above example, it is likely called with `session_module_name()`. e.g. session_module_name('memcache'); session_serializer_name('igbinary'); It looks better than session_module_name('memcached'); ini_set('session.serializer_handler', 'igbinary'); It's reasonable to have session_serializer_name(), since it would be used with `session_module_name()`. API should have good name here, too. We may discuss what we should do with module functions that modify it's INI. There are number of them already. I think you need RFC for this. My opinion is "There should be API with obvious name, if it is used often or mandatory". Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73078 — Andrey Andreev — 2014-03-12T11:04:03+00:00 URL: https://externals.io/message/73066#73078 · In reply to: #73071 Hi, > Hi Andrey, > > > > […] > > > Of course you can. > It does not look nice as API. API should have good name for it. IMO. > > `session_gc()` is useful for both low traffic site and high traffic site. > I don't see reason not to have explicit gc API. User may simply call > > `session_gc()`; > > periodically by cron, etc. There's a very good reason not to have it, you just ignored most of my previous email which explained exactly that. > > […] > > > Those aren't changed by programs often. Oh, trust me - cookie lifetime is changed way more often than the serializer. And how do you define often anyway? Sessions are something that are setup once, it's rarely the case that a programmer needs to change any of the session INI settings more than once in the same program. > As I wrote, 3rd party save handler implements their own serializers. > Not like above example, it is likely called with `session_module_name()`. e.g. > > session_module_name('memcache'); > session_serializer_name('igbinary'); It's your opinion that this is "likely" done that way. I for one have *never* used `session_module_name()`. And again, this is not something that people would write more often than once in the whole application. > It looks better than > > session_module_name('memcached'); > ini_set('session.serializer_handler', 'igbinary'); It doesn't look better than: ini_set('session.save_handler', 'memcached'); ini_set('session.serialize_handler', 'igbinary'); IMO, this is more explicit and more understandable for people who haven't read the manuals for the "custom" functions. > It's reasonable to have session_serializer_name(), since it would be used > with `session_module_name()`. API should have good name here, too. I'm again questioning whether `session_module_name()` is useful/necessary in the first place. > We may discuss what we should do with module functions that modify it's INI. > There are number of them already. I think you need RFC for this. On this, I agree. > My opinion is "There should be API with obvious name, if it is used often or > mandatory". On this - not. ;) Cheers, Andrey. --- ## #73080 — Yasuo Ohgaki — 2014-03-12T11:30:03+00:00 URL: https://externals.io/message/73066#73080 · In reply to: #73078 Hi Andrey, > > > […] > wrote: > > […] > This > > […] > > There's a very good reason not to have it, you just ignored most of my > previous email which explained exactly that. > Users may abuse INI or API. If you worry about abuse, it would be better to have API and warn users. Users should purge deleted session data for security reasons. It is especially important for low traffic sites. High traffic sites may need periodic purge due varying request rate. Not having GC API is API design bug. If there should not be convenient API, file_get/put_contents() aren't needed neither. > > […] > > Oh, trust me - cookie lifetime is changed way more often than the > serializer. > It's bad practice, indeed. User should not change default 0 (session cookie - i.e. cookie only lives in browser's memory and destroyed by browser termination) If users need auto login or like, they should implement it properly by using `setcookie()`. (i.e. Never use session ID cookie for long life session) And how do you define often anyway? Sessions are something that are > setup once, it's rarely the case that a programmer needs to change any > of the session INI settings more than once in the same program. > > > […] > e.g. > > […] > > It's your opinion that this is "likely" done that way. I for one have > *never* used `session_module_name()`. > And again, this is not something that people would write more often > than once in the whole application. > It does not matter much if you use often or not. As an API design, not having session_serializer_name() is strange. > > > […] > > It doesn't look better than: > > ini_set('session.save_handler', 'memcached'); > ini_set('session.serialize_handler', 'igbinary'); > I don't mind having discussion deprecating all API that modifying its INI. I'm some where between +1 and 0 for this. IMO, this is more explicit and more understandable for people who > haven't read the manuals for the "custom" functions. > > > […] > > I'm again questioning whether `session_module_name()` is > useful/necessary in the first place. > Now I think I understand your point of view. > We may discuss what we should do with module functions that modify it's > INI. > > […] > > On this, I agree. > Consistency matters. I'm trying to be consistent with current way. I'm tends to agree deprecating all INI modifying APIs ;) Let's have the RFC, I'll vote +1 for it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73083 — Andrey Andreev — 2014-03-12T12:03:10+00:00 URL: https://externals.io/message/73066#73083 · In reply to: #73080 Hi again, > Hi Andrey, > > > > […] > > > Users may abuse INI or API. > If you worry about abuse, it would be better to have API and warn users. I don't think you understand what my point is ... the API itself must be hard to abuse. The INI settings are not that often misused because they are hard to find for anybody that's not specifically looking for them. This is not the case for functions and exposing the GC as a function is something that would at least triple the abuse. > Users should purge deleted session data for security reasons. It is > especially important for low traffic sites. High traffic sites may need > periodic purge due varying request rate. I don't disagree with that, but it's not what we're discussing. > Not having GC API is API design bug. There is an API, you're introducing a second one. > If there should not be convenient API, file_get/put_contents() aren't needed > neither. Nonsense. This is not even worth explaining why. > > […] > > > It's bad practice, indeed. > User should not change default 0 (session cookie - i.e. cookie only lives in > browser's memory and destroyed by browser termination) > > If users need auto login or like, they should implement it properly by using > `setcookie()`. (i.e. Never use session ID cookie for long life session) I was just giving you an example of how your assumption is wrong, auto-login is not the subject of this discussion. > > […] > > > It does not matter much if you use often or not. > As an API design, not having session_serializer_name() is strange. You just changed your argumentation from "it's often needed" to "it doesn't matter if it's often used, but not having it is strange". Again, my stance is that the existance `session_module_name()` is strange, not the other way around. > > […] > > > I don't mind having discussion deprecating all API that modifying its INI. > I'm some where between +1 and 0 for this. > > > […] > > > Now I think I understand your point of view. > > > […] > > > Consistency matters. I'm trying to be consistent with current way. > > I'm tends to agree deprecating all INI modifying APIs ;) > Let's have the RFC, I'll vote +1 for it. So ... you'd vote +1 for deprecating all similar functions, yet you just added another one without even discussing it? :) That would be a nice RFC indeed, but I still stand behind my initial suggestion - both of these functions should be excluded from the 5.6 release. Cheers, Andrey. --- ## #73086 — Julien Pauli — 2014-03-12T16:00:44+00:00 URL: https://externals.io/message/73066#73086 · In reply to: #73066 > Hello, > > I just saw this in the 5.6-alpha1 changelog: > > - Implemented Request #54649 (Create session_serializer_name()). (Yasuo) > ... > - Implemented Request #11100 (session_gc() function). (Yasuo) > > > I didn't find an RFC and I don't know a way to search for a discussion > on internals (Google doesn't find one). > > session_serializer_name(): https://bugs.php.net/bug.php?id=54649 > > I don't think that this should've been implemented, especially with > the reasoning from the feature request ... The same could be applied > to all session ini options and especially now since `session_start()` > accepts them, it's useless IMO. > > `session_gc()` was added in a similar fashion: > https://bugs.php.net/bug.php?id=11100 > > There's already a comment about how the same thing can be achieved by > overloading SessionHandler::gc(), but a user could also just alter > session.gc_divisor, session.gc_probability to ensure that the garbage > collector is started. > And while session_serializer_name() is just redundant, `session_gc()` > could cause performance issues. > > I'd strongly suggest that these 2 functions should be removed before > PHP 5.6 is officially released. > > Regards, > Andrey Andreev. > > -- > > > > Hello guys, When we branched 5.6, we branched it from master, with these commits into it. Ferenc did a hard job to spot what was in master at this time, and it looks like he didn't notice those addings. We should revert them. We *cannot* accept new functions in 5.6 , if the community in its whole, does not agree with them, by voting an RFC. I'm sorry, but that is our policy, and we are not talking about "little" functions we could blindly accept (because we are not bad dragons neither :-p, some things are tiny and can be merged without heavy RFC process). Those changes impact the session module, which is an important one. Yasuo : I know you are actually working on RFCs for sessions for 5.6. I would love to see them accepted and merged to 5.6. From what I know, one has already been accepted partially ( https://wiki.php.net/rfc/session-lock-ini ). It seems like it's not been merged yet ? Is it because it cant, like you seem to say in http://markmail.org/message/dcykcmq6exvkflf7 ? Please, also remember that we are going to tag beta1 on 18th , your RFCs need to be accepted and merged for this date. Having said that, I agree we should clean the session API. I agree `session_module_name()` is very uncommon and likely not very used, and it should dissapear. I agree as well that having to touch INI settings with `ini_set()` to change the session behavior is not really nice, and that those should be feasable using functions, or arguments to `session_start()`; However, all those points are BC breaking points, and can not be addressed for 5.6 ; but I encourage all of you to start discussions about the session module and those points, for PHP6. Thank you all for your understanding, we all want to make PHP better, but we all are humans :-p Julien.Pauli --- ## #73087 — Yasuo Ohgaki — 2014-03-12T21:12:35+00:00 URL: https://externals.io/message/73066#73087 · In reply to: #73083 Hi Andrey, > > > […] > wrote: > > […] > wrote: > > […] > site. > > […] > > I don't think you understand what my point is ... the API itself must > be hard to abuse. > The INI settings are not that often misused because they are hard to > find for anybody that's not specifically looking for them. This is not > the case for functions and exposing the GC as a function is something > that would at least triple the abuse. > > > […] > > I don't disagree with that, but it's not what we're discussing. The root cause is design bug of GC. We may fix this with other method. This would be the best solution. It's possible that save handlers use session.expire setting to execute GC periodically. > > […] > lives in > > […] > using > > […] > > I was just giving you an example of how your assumption is wrong, > auto-login is not the subject of this discussion. This is the common abuse of session. IMO. > > […] > `session_module_name()`. > > […] > > You just changed your argumentation from "it's often needed" to "it > doesn't matter if it's often used, but not having it is strange". > Again, my stance is that the existance `session_module_name()` is > strange, not the other way around. > "often or mandatory" I think it's both. > > > […] > INI. > > […] > it's > > […] > > So ... you'd vote +1 for deprecating all similar functions, yet you > just added another one without even discussing it? :) > > That would be a nice RFC indeed, but I still stand behind my initial > suggestion - both of these functions should be excluded from the 5.6 > release. No problem. It would be easier to understand if you explain your point ;) I'll fix root cause of GC issue later. GC should not depend on luck. Are you going to write RFC for INI issue? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73088 — Andrey Andreev — 2014-03-12T23:43:32+00:00 URL: https://externals.io/message/73066#73088 · In reply to: #73087 Hi Yasuo, > I'll fix root cause of GC issue later. GC should not depend on luck. What do you mean you'll fix it? What would the solution to that be? > Are you going to write RFC for INI issue? Not at this time, it won't make it into 5.6 anyway. Julien Pauli already suggested that we should discuss that for PHP6 ... I have some ideas, I'll start a discussion about them soon. :) Cheers, Andrey. --- ## #73093 — Yasuo Ohgaki — 2014-03-13T01:40:07+00:00 URL: https://externals.io/message/73066#73093 · In reply to: #73088 Hi Andrey, > > […] > > What do you mean you'll fix it? What would the solution to that be? Save handler may keep & check last GC time. If it's over session.expire, perform GC. Issue is that "session data that should be deleted" is not deleted and could be alive long term due to current design. Current design is good enough for many usage, but it is not the best practice obviously. "Keep & check last GC time" requires additional overhead for every request. This is the reason why I prefer to have `session_gc()` and encourage users to use it properly. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73094 — Yasuo Ohgaki — 2014-03-13T01:49:00+00:00 URL: https://externals.io/message/73066#73094 · In reply to: #73088 Hi Andrey, > > […] > > Not at this time, it won't make it into 5.6 anyway. > Julien Pauli already suggested that we should discuss that for PHP6 > ... I have some ideas, I'll start a discussion about them soon. :) I think it good to document them now. I don't mind at all adding `E_DEPRECATED` for `session_module_name()`, `mb_regex_encoding()`, etc for PHP 5.7+. Could you create the RFC? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73096 — Yasuo Ohgaki — 2014-03-13T03:03:00+00:00 URL: https://externals.io/message/73066#73096 · In reply to: #73094 Hi Andrey, > > > > […] > > > I think it good to document them now. > I don't mind at all adding `E_DEPRECATED` for `session_module_name()`, > `mb_regex_encoding()`, etc for PHP 5.7+. > > Could you create the RFC? > I've created RFC https://wiki.php.net/rfc/deprecate-ini-functions Please feel free to update it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73098 — Patrick Schaaf — 2014-03-13T07:15:00+00:00 URL: https://externals.io/message/73066#73098 · In reply to: #73093 Am 13.03.2014 02:41 schrieb "Yasuo Ohgaki" : > > Save handler may keep & check last GC time. If it's over session.expire, > perform GC. That would have a "scoping" problem: session.expire, at least in theory, can be different for different scripts / usages of session in the same application, but such a global last-GC timestamp would be... global. Also generally GC is a costly global operation, slowing down the request that hits it (probably that's why you envision a cronjob right?) > Issue is that "session data that should be deleted" is not deleted and > could be alive long term due to current design. Current design is good The _proper_ solution, would be to keep a timestamp stored with each session. kind of a last-modified timestamp. (expiry does work off last modification, right?). Anyhow, the trick is to check at session_start whether that timestamp plus whatever session.expire is set at, already is in the past - and when that happens, destroy the one session on storage and act as if it hadn't been found. This way there is no longer any correctness problem, and a GC is only needed to clean up sessions that are never requested again - and for that I fully agree that a separate - e.g. cron driven - "GC now" job would be best (not affecting latency of running frontent requests too much). Of course, going to such a solution, requires a change to all session implementations in that they must provide for storage and return of such a last-modified timestamp (or othewise the improvement cannot be realized for a non-updated-to-the-new-scheme session handler) best regards Patrick --- ## #73099 — Yasuo Ohgaki — 2014-03-13T07:45:17+00:00 URL: https://externals.io/message/73066#73099 · In reply to: #73098 Hi Patrick, > > […] > > That would have a "scoping" problem: session.expire, at least in theory, > can be different for different scripts / usages of session in the same > application, but such a global last-GC timestamp would be... global. > > Also generally GC is a costly global operation, slowing down the request > that hits it (probably that's why you envision a cronjob right?) > Yes. This is issue for save handlers that should perform costly GC. > > […] > > The _proper_ solution, would be to keep a timestamp stored with each > session. kind of a last-modified timestamp. (expiry does work off last > modification, right?). Anyhow, the trick is to check at session_start > whether that timestamp plus whatever session.expire is set at, already is > in the past - and when that happens, destroy the one session on storage and > act as if it hadn't been found. > > This way there is no longer any correctness problem, and a GC is only > needed to clean up sessions that are never requested again - and for that I > fully agree that a separate - e.g. cron driven - "GC now" job would be best > (not affecting latency of running frontent requests too much). > > Of course, going to such a solution, requires a change to all session > implementations in that they must provide for storage and return of such a > last-modified timestamp (or othewise the improvement cannot be realized for > a non-updated-to-the-new-scheme session handler) > I agree. It's proper and feasible solution. I've sent issues about `session_regenerate_id()` in other thread. It could be solved by time stamp stored as session data also. Storing time stamp out side of $_SESSION data would be cleanest solution, but it requires new interface. It may be good idea to write a new save handler interface while keeping older one. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73103 — Pierre Joye — 2014-03-13T09:22:01+00:00 URL: https://externals.io/message/73066#73103 · In reply to: #73099 > It may be good idea to write a new save handler interface while keeping > older one. It would really helpful to target a later versions, maybe 6 and see what we can do while minimizing the impact in userlands (whether or not custom session handlers are implemented). But all the discussions and RFCs about basically the same needs have been very confusing. What do you think? Is it something you could consider? -- Pierre @pierrejoye | http://www.libgd.org --- ## #73104 — Lester Caine — 2014-03-13T10:04:05+00:00 URL: https://externals.io/message/73066#73104 · In reply to: #73098 Patrick Schaaf wrote: > This way there is no longer any correctness problem, and a GC is only > needed to clean up sessions that are never requested again - and for that I > fully agree that a separate - e.g. cron driven - "GC now" job would be best > (not affecting latency of running frontent requests too much). I've not had much interest in this discussion as I am more than happy with how things work now. It is not unusual for my 'client' sessions to last for an hour or so without activity and their 'automatically' logging out was the problem originally. So now I'm set never to clear session data and run a clear up on the session directory before the site starts work in the morning. There are reasons a system wide approach may be more practical than changing how a single session handles things? -- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk --- ## #73106 — Andrey Andreev — 2014-03-13T10:36:35+00:00 URL: https://externals.io/message/73066#73106 · In reply to: #73103 Hi, > The _proper_ solution, would be to keep a timestamp stored with each session. kind of a last-modified timestamp. (expiry does work off last modification, right?). Anyhow, the trick is to check at session_start whether that timestamp plus whatever session.expire is set at, already is in the past - and when that happens, destroy the one session on storage and act as if it hadn't been found. Such a timestamp already exists, you can't implement sessions without one because there would be no way to know when a session have expired. For the files handler, it's `filemtime()` (afaik) and the behavior that you described is probably already in use (I see no reason not to). However, that's not in the scope of GC. > a GC is only needed to clean up sessions that are never requested again - and for that I fully agree that a separate - e.g. cron driven - "GC now" job would be best (not affecting latency of running frontent requests too much). ^ That is what a garbage collector is, indeed. :) > > > > […] > > > It would really helpful to target a later versions, maybe 6 and see > what we can do while minimizing the impact in userlands (whether or > not custom session handlers are implemented). > > But all the discussions and RFCs about basically the same needs have > been very confusing. > > What do you think? Is it something you could consider? I am deffinately a proponent of that. I already mentioned earlier that I have some ideas ... this is one of them. However, while Yasuo seems to always be keen on creating RFCs immediately, I'd rather focus on the existing ones that are session related: https://wiki.php.net/rfc/secure-session-options-by-default https://wiki.php.net/rfc/session_regenerate_id 5.6 is really close and this is important stuff that was supposed to go in, but has not had updates recently and lagged behind. Changes for future versions can wait a few days/weeks ... Cheers, Andrey. --- ## #73112 — Andrey Andreev — 2014-03-13T16:56:11+00:00 URL: https://externals.io/message/73066#73112 · In reply to: #73106 Hi again, Another 2 items that didn't go through the RFC process or any discussion: - Implemented Request #20421 (session_abort() and `session_reset()` function). (Yasuo) https://bugs.php.net/bug.php?id=20421 Does this change mtime of the session? I also have other (rather philosophical) arguments against this, but I don't want to get deeper into it now ... it should just be reverted together with session_serializer_name(), `session_gc()` on the basis of no discussion at all. - Implemented Request #17860 (Session write short circuit). (Yasuo) https://bugs.php.net/bug.php?id=17860 The problem with that is that there's no API exposed for custom save handlers to decide when to only update mtime or the whole session. I think the question was exactly about that, actually. It is also not clear how is that implemented and for which of the available session handlers ... is it all of them? Then later (note that it *already is implemented*), it was voted as part of this RFC, which made it optional: https://wiki.php.net/rfc/session-lock-ini This RFC is broken to begin with ... what ended up as accepted from this RFC was actually very far from the main intention behind it (and it silently accepted passing ini options to `session_start()` with no explicit voting for that - just saying, otherwise I like that feature). Also, there's no description for "read_only" except the obvious - that it only reads. It should at least mention that a shared lock can be used for that case. IMO, if this feature had gone through proper discussion instead of just presenting a yes/no option as part of another change, we'd end up with a different solution. Actually, I'd like to ask if it's possible for an RFC to override a decision taken from a previous one? And can it be done quick? Or if not - can that be postponed for 5.7? The lazy_write option is nice, but it's not optimal and it would be embarassing if it makes it into 5.6 only to be later reversed in favor of a better solution. What I'm thinking of is a session_is_changed() function and "lazy_write" at all times (there's no reason for that to be optional). Regards, Andrey. --- ## #73120 — Yasuo Ohgaki — 2014-03-13T19:32:13+00:00 URL: https://externals.io/message/73066#73120 · In reply to: #73106 Hi Andrey, > > […] > session. kind of a last-modified timestamp. (expiry does work off last > modification, right?). Anyhow, the trick is to check at session_start > whether that timestamp plus whatever session.expire is set at, already is > in the past - and when that happens, destroy the one session on storage and > act as if it hadn't been found. > > Such a timestamp already exists, you can't implement sessions without > one because there would be no way to know when a session have expired. > For the files handler, it's `filemtime()` (afaik) and the behavior that > you described is probably already in use (I see no reason not to). > Time stamp exists. It's correct partially. The time stamp is not usable for HTTP session manager to manage session properly. We may have new API that gets time stamp of session data, but it would require large overhead. i.e. Need additional API call to get time stamp = additional query to session storage. The overhead cannot be ignored. The proper way to get and set the time stamp is "get/set time stamp" when "store/retrieve session data". This way, there would not be overhead. User may implement in their script, but it's a session manager task to begin with. > > However, that's not in the scope of GC. > Managing time stamp is the main task of session GC, isn't it? > > > […] > - and for that I fully agree that a separate - e.g. cron driven - "GC now" > job would be best (not affecting latency of running frontent requests too > much). > > ^ That is what a garbage collector is, indeed. :) > > On Thu, Mar 13, 2014 at 11:22 AM, Pierre Joye > wrote: > > […] > wrote: > > […] > > I am deffinately a proponent of that. I already mentioned earlier that > I have some ideas ... this is one of them. > > However, while Yasuo seems to always be keen on creating RFCs > immediately, I'd rather focus on the existing ones that are session > related: You are the one who said there should be RFCs ;) There are RFCs because I'm trying to change existing behaviors rather than simple bug fixes. Anyway, this was discussion of committed patch. It has much higher priority than not implemented RFC, I suppose. I still think there should be GC function. Programmers should control GC and there should be session function for it rather than INI tweak. https://wiki.php.net/rfc/secure-session-options-by-default > https://wiki.php.net/rfc/session_regenerate_id > > 5.6 is really close and this is important stuff that was supposed to > go in, but has not had updates recently and lagged behind. > Changes for future versions can wait a few days/weeks ... It's better to write something so that it would be forgotten. I forgot to update session_regenerate_id RFC since it became part of other RFC. I'll update it soon. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73125 — Yasuo Ohgaki — 2014-03-13T20:09:01+00:00 URL: https://externals.io/message/73066#73125 · In reply to: #73112 Hi Andrey, > Another 2 items that didn't go through the RFC process or any discussion: > These are covered RFC discussions and/or some other discussions in this list. > - Implemented Request #20421 (session_abort() and `session_reset()` > function). (Yasuo) > > https://bugs.php.net/bug.php?id=20421 > > Does this change mtime of the session? > I also have other (rather philosophical) arguments against this, but I > don't want to get deeper into it now ... it should just be reverted > together with session_serializer_name(), `session_gc()` on the basis of > no discussion at all. > abort and reset were discussed in RFC discussion. It was't a long discussion though. Since this is just a simple API addition, it added simply. What's your reason? > > - Implemented Request #17860 (Session write short circuit). (Yasuo) > > https://bugs.php.net/bug.php?id=17860 > > The problem with that is that there's no API exposed for custom save > handlers to decide when to only update mtime or the whole session. I > think the question was exactly about that, actually. > It is also not clear how is that implemented and for which of the > available session handlers ... is it all of them? > This one is discussed in following RFC, since it affects existing behavior. Some one pointed out and the RFC was created. Then later (note that it *already is implemented*), it was voted as > part of this RFC, which made it optional: > > https://wiki.php.net/rfc/session-lock-ini > > This RFC is broken to begin with ... what ended up as accepted from > this RFC was actually very far from the main intention behind it (and > it silently accepted passing ini options to `session_start()` with no > explicit voting for that - just saying, otherwise I like that > feature). > Also, there's no description for "read_only" except the obvious - that > it only reads. It should at least mention that a shared lock can be > used for that case. > > IMO, if this feature had gone through proper discussion instead of > just presenting a yes/no option as part of another change, we'd end up > with a different solution. > > Actually, I'd like to ask if it's possible for an RFC to override a > decision taken from a previous one? And can it be done quick? Or if > not - can that be postponed for 5.7? > The lazy_write option is nice, but it's not optimal and it would be > embarassing if it makes it into 5.6 only to be later reversed in favor > of a better solution. What I'm thinking of is a session_is_changed() > function and "lazy_write" at all times (there's no reason for that to > be optional). > You should comment during discussion. There was long discussion for this. Shared lock could be used easily for some session storage, but it requires save handlers modification and it could be implemented _independent_ of read_only. Web apps must return response ASAP. Therefore, any locks that could be avoided should be able to be avoided including read(shared) lock. Anyway, read_lock is confusing unless save handler API supports it. i.e. User cannot know if it really support read_lock like current use_strict_mode. Why not write a RFC for read_lock? I'm OK to implement it if you would like. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73127 — Andrey Andreev — 2014-03-13T20:29:29+00:00 URL: https://externals.io/message/73066#73127 · In reply to: #73120 > Hi Andrey, > > > > […] > > > Time stamp exists. It's correct partially. > > The time stamp is not usable for HTTP session manager to manage session > properly. We may have new API that gets time stamp of session data, but it > would require large overhead. i.e. Need additional API call to get time > stamp = additional query to session storage. The overhead cannot be ignored. > > The proper way to get and set the time stamp is "get/set time stamp" when > "store/retrieve session data". This way, there would not be overhead. User > may implement in their script, but it's a session manager task to begin > with. > > > […] > > > Managing time stamp is the main task of session GC, isn't it? Yasuo, you took 2 paragraphs that belong together and put them out of context ... > > […] > > > You are the one who said there should be RFCs ;) > There are RFCs because I'm trying to change existing behaviors rather than > simple bug fixes. > > > Anyway, this was discussion of committed patch. It has much higher priority > than not implemented RFC, I suppose. I still think there should be GC > function. Programmers should control GC and there should be session function > for it rather than INI tweak. Exactly, focus on current tasks is what I'm saying. In the timespan of 5 hours you asked me twice if I will write the RFC _and_ wrote it without even waiting for the reply. Let's stop this nonsense now and discuss more important stuff. :) Cheers, Andrey. --- ## #73131 — Andrey Andreev — 2014-03-13T21:01:24+00:00 URL: https://externals.io/message/73066#73131 · In reply to: #73125 > Hi Andrey, > > > > […] > > > These are covered RFC discussions and/or some other discussions in this > list. > > > […] > > > abort and reset were discussed in RFC discussion. It was't a long > discussion though. Since this is just a simple API addition, it added > simply. > > What's your reason? http://markmail.org/message/om3ofpjlit7iq6d2 Is this the discussion you're talking about? If so, `session_abort()` and `session_reset()` are barely mentioned in it, and no wonder - it was during the holidays. That's not a productive discussion ... > > […] > > > This one is discussed in following RFC, since it affects existing behavior. > Some one pointed out and the RFC was created. The RFC mentions a 'lazy_write' option and voting ended at the end of February. The feature (not optional) was committed way earlier. > > […] > > > You should comment during discussion. There was long discussion for this. Sorry, I wasn't around during that discussion. That's why I'm asking if a new RFC can override the old one. > Shared lock could be used easily for some session storage, but it requires > save handlers modification and it could be implemented _independent_ of > read_only. Web apps must return response ASAP. Therefore, any locks that > could be avoided should be able to be avoided including read(shared) lock. I'm talking about handlers that PHP itself ships, they all implement locking (exclusive locking). An exclusive lock can only be obtained by one process and all others will have to wait for it to be freed. A shared lock can be acquired by multiple processes and as such, is concurrency-friendly. However, it is only usable if sessions are in read_only mode, hence why I'm suggesting it. > Anyway, read_lock is confusing unless save handler API supports it. i.e. > User cannot know if it really support read_lock like current > use_strict_mode. Users needs to know if they are in read_only mode. Currently they can't, because the RFC proposed it an obscure setting available only to `session_start()`. If it was an INI setting, it could've been readable. > Why not write a RFC for read_lock? I'm OK to implement it if you would like. Because this is one of the few session-related things that doesn't need an RFC, but rather common sense. It's just an implementation detail that was not described in the RFC. ---------- All I'm saying is, this whole thing wasn't handled properly and/or with the required attention. It's nobody's fault, but it ended up broken and I want to fix it (possibly with the help of somebody to write the patch, because I'm not a C programmer). I'll write an RFC on it shortly and I hope it can get through before the release. Cheers, Andrey. --- ## #73132 — Yasuo Ohgaki — 2014-03-13T22:01:10+00:00 URL: https://externals.io/message/73066#73132 · In reply to: #73131 Hi Andrey, > > […] > requires > > […] > lock. > > I'm talking about handlers that PHP itself ships, they all implement > locking (exclusive locking). > > An exclusive lock can only be obtained by one process and all others > will have to wait for it to be freed. > A shared lock can be acquired by multiple processes and as such, is > concurrency-friendly. However, it is only usable if sessions are in > read_only mode, hence why I'm suggesting it. > read_only mode is "read and finish session". Shared lock is "can read session, but not write". Semantics differs. > > > […] > > Users needs to know if they are in read_only mode. > Currently they can't, because the RFC proposed it an obscure setting > available only to `session_start()`. If it was an INI setting, it > could've been readable. > I like INIs. Therefore, it was INI initially. It was made to start option, since others do not like INIs. > Why not write a RFC for read_lock? I'm OK to implement it if you would > like. > > Because this is one of the few session-related things that doesn't > need an RFC, but rather common sense. > It's just an implementation detail that was not described in the RFC. > My common sense tells that there should be `session_gc()`, there should be session_serializer_name() if there is session_module_name ;) However, I like the idea of getting rid of INI modifying functions. I'm OK with either - have both `session_module_name()` and session_serializer_name() - getting rid of them all The RFC was made with long discussions. It involves other RFC, too. I may miss to write some and it may be difficult to understand reason behind it. I'll answer if there is questions. Please don't forget to write your reason why feature should be changed. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73133 — Yasuo Ohgaki — 2014-03-13T22:18:48+00:00 URL: https://externals.io/message/73066#73133 · In reply to: #73131 Hi Andrey, > All I'm saying is, this whole thing wasn't handled properly and/or > with the required attention. > It's nobody's fault, but it ended up broken and I want to fix it > (possibly with the help of somebody to write the patch, because I'm > not a C programmer). > I think shared lock is useful. It requires additional work to change save handler interface and save handler code. It's a distinct feature from read_only. You can get status via `session_status()` if you need to know during execution. i.e. If session is started with read_only, you'll get `PHP_SESSION_NONE` status. I suppose read_only is good enough name for the feature. I appreciate better name for it, if there is. Although I would not like to propose/implement the RFC now, but I may implement it if RFC passes. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- Next page: https://externals.io/message/73066.md?page=2