# session_reset() and session_abort() to send errors - Thread: https://externals.io/message/73338 - Original archive: https://news-web.php.net/php.internals/73338 - Started by Julien Pauli on 2014-03-20T16:01:40+00:00 - Messages: 15 - 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 --- ## #73338 — Julien Pauli — 2014-03-20T16:01:40+00:00 URL: https://externals.io/message/73338#73338 Hi, I was just reviewing the new code and functions in 5.6 for session module. I noticed that `session_reset()` and `session_abort()` came in. I would suggest those functions to throw errors when the session is not initialized. This is already done for all other session related functions (session_reset_id(), `session_regenerate_id()`, `session_destroy()`). Thoughts ? Julien Pauli --- ## #73350 — Yasuo Ohgaki — 2014-03-20T23:57:59+00:00 URL: https://externals.io/message/73338#73350 · In reply to: #73338 Hi Julien, > I was just reviewing the new code and functions in 5.6 for session module. > > I noticed that `session_reset()` and `session_abort()` came in. > > I would suggest those functions to throw errors when the session is not > initialized. > > This is already done for all other session related functions > (session_reset_id(), `session_regenerate_id()`, `session_destroy()`). > > Thoughts ? > I agree and they should. I'll take care of them or anyone else? It will be few days later. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73354 — Julien Pauli — 2014-03-21T10:40:58+00:00 URL: https://externals.io/message/73338#73354 · In reply to: #73350 > Hi Julien, > > > > > […] > > I agree and they should. > I'll take care of them or anyone else? > It will be few days later. > You can go for it, I have no git connection today :-p Thank you ! Julien --- ## #73428 — Andrey Andreev — 2014-03-25T16:06:29+00:00 URL: https://externals.io/message/73338#73428 · In reply to: #73354 Hi, I had previously requested the removal of these 2 functions based on lack of discussion about them (or at least I found none). My request was ignored because I had to tell "why it's not needed", which I don't think is the right approach for a new feature that somebody just decided to commit, but regardless ... here's a good reason: From what I understand, `session_reset()` not only discards changes to $_SESSION and not just re-reads the data, but re-initializes the whole session. This includes calls to open(), read(), which include opening file pointers, setting locks, checking session ID validity, etc. I don't know how safe that is to do internally, but with a userland session handler, it means discarding previously open connections/file pointers, locks and whoever knows what else a developer might've cached, assuming that open(), read() would only be called once. It would've also been better IMO if the function was called session_restart() instead, because of the above-described behavior. I also don't like that these 2 functions kind of make sessions to look like a traditional database transaction, or at least I'd hate it to see code that uses them for the wrong purpose, but that's rather philosophical ... I do see them as useful for resetting the application to a certain state in case of an error, but in that context - `session_abort()` alone should be sufficient. Cheers, Andrey. --- ## #73442 — Julien Pauli — 2014-03-26T09:57:58+00:00 URL: https://externals.io/message/73338#73442 · In reply to: #73428 > Hi, > > I had previously requested the removal of these 2 functions based on > lack of discussion about them (or at least I found none). My request > was ignored because I had to tell "why it's not needed", which I don't > think is the right approach for a new feature that somebody just > decided to commit, but regardless ... here's a good reason: > > From what I understand, `session_reset()` not only discards changes to > $_SESSION and not just re-reads the data, but re-initializes the whole > session. This includes calls to open(), read(), which include opening > file pointers, setting locks, checking session ID validity, etc. I > don't know how safe that is to do internally, but with a userland > session handler, it means discarding previously open connections/file > pointers, locks and whoever knows what else a developer might've > cached, assuming that open(), read() would only be called once. > It would've also been better IMO if the function was called > session_restart() instead, because of the above-described behavior. > > I also don't like that these 2 functions kind of make sessions to look > like a traditional database transaction, or at least I'd hate it to > see code that uses them for the wrong purpose, but that's rather > philosophical ... I do see them as useful for resetting the > application to a certain state in case of an error, but in that > context - `session_abort()` alone should be sufficient. > Yup, I agree that I dont really see a use case for `session_reset()`. The implementation looks strange. Yasuo, could you clarify please ? Julien --- ## #73452 — Yasuo Ohgaki — 2014-03-27T10:38:16+00:00 URL: https://externals.io/message/73338#73452 · In reply to: #73442 Hi all, > Yup, I agree that I dont really see a use case for `session_reset()`. > The implementation looks strange. > > Yasuo, could you clarify please ? > This function is useful for session save handlers that do not lock session data. Example is memcached. This function could be used re-read session data to mitigate over written session data with unlock session data. Andrey, just because you don't think of usage, it does not mean it does not mean unneeded or not useful. You are better to ask the reason behind why first. Good library should have defined API for specific tasks, too. You also has misunderstanding about why delayed deletion for `session_regenerate_id()` is mandatory. BTW, raising errors for invalid calls has issues, so I send PR to discuss. https://github.com/php/php-src/pull/634 Regards, P.S. If I should choose to remove this or `session_gc()`, I choose this because `session_gc()` is mandatory API for decent session manager. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73454 — Andrey Andreev — 2014-03-27T12:05:57+00:00 URL: https://externals.io/message/73338#73454 · In reply to: #73452 Hi, > This function is useful for session save handlers that do not lock > session data. Example is memcached. This function could be used > re-read session data to mitigate over written session data with unlock > session data. > > Andrey, just because you don't think of usage, it does not mean it does not > mean unneeded or not useful. You are better to ask the reason behind why > first. > Good library should have defined API for specific tasks, too. You also has > misunderstanding about why delayed deletion for `session_regenerate_id()` > is mandatory. Yasuo ... There's no misunderstanding, I just have a disagreement with you about that issue and I'm tired of arguing over that. Let's just keep `session_regenerate_id()` in it's own thread, ok? But speaking of mandatory: locks are mandatory, so how are non-locking handlers an argument in all of this? Also, I didn't just question if there's a use case, I'm questioning if it is safe to use `session_reset()` at all. Re-reading doesn't imply discarding currently open resources and complerely re-initializing the session and from what I see - that's exactly what the function does, or am I missing something? Cheers, Andrey. --- ## #73456 — Yasuo Ohgaki — 2014-03-28T06:48:54+00:00 URL: https://externals.io/message/73338#73456 · In reply to: #73454 Hi Andrey, > > […] > not > > […] > has > > […] > > Yasuo ... There's no misunderstanding, I just have a disagreement with > you about that issue and I'm tired of arguing over that. Let's just > keep `session_regenerate_id()` in it's own thread, ok? > Ok. I'm talking about XHR cannot be solution and delayed deletion is mandatory for precise session management. We may discuss later since I'm going to fork session module. > But speaking of mandatory: locks are mandatory, so how are non-locking > handlers an argument in all of this? > I use unlocked session for performance tuning often. Apps could be much faster with little care and little code changes. Sometimes this function is needed to be sure. Also, I didn't just question if there's a use case, I'm questioning if > it is safe to use `session_reset()` at all. Re-reading doesn't imply > discarding currently open resources and complerely re-initializing the > session and from what I see - that's exactly what the function does, > or am I missing something? I think session manager should "manage" how session is managed. Save handlers should save/retrieve session data only. Since session manager does not have such feature, save handlers does the task and there is unlocked sessions. Since there is no exposed user functions for save handlers, reset is needed on occasion. I would rather have `session_gc()` back as I wrote in previous mail, though. This one is truly mandatory function even with precise session data expiration. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73457 — Yasuo Ohgaki — 2014-03-28T06:54:22+00:00 URL: https://externals.io/message/73338#73457 · In reply to: #73428 Hi Andrey, > From what I understand, `session_reset()` not only discards changes to > $_SESSION and not just re-reads the data, but re-initializes the whole > session. This includes calls to open(), read(), which include opening > file pointers, setting locks, checking session ID validity, etc. I > don't know how safe that is to do internally, but with a userland > session handler, it means discarding previously open connections/file > pointers, locks and whoever knows what else a developer might've > cached, assuming that open(), read() would only be called once. > It would've also been better IMO if the function was called > session_restart() instead, because of the above-described behavior. > It's safe. Don't worry. As always, I prefer better names. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73466 — Andrey Andreev — 2014-03-28T09:41:05+00:00 URL: https://externals.io/message/73338#73466 · In reply to: #73457 Hi, > Hi Andrey, > > > > […] > > > It's safe. Don't worry. > As always, I prefer better names. Well, I just re-checked it and does indeed just call php_session_initialize(), so what's the point? Shorthand for `session_abort()` && `session_start()` ? Cheers, Andrey. --- ## #73495 — Yasuo Ohgaki — 2014-04-01T00:59:48+00:00 URL: https://externals.io/message/73338#73495 · In reply to: #73466 Hi Andrey, > > > […] > wrote: > > […] > > Well, I just re-checked it and does indeed just call > php_session_initialize(), so what's the point? Shorthand for > `session_abort()` && `session_start()` ? Yes, it's an API for it. It's more efficient than user land functions as it requires needless close and open (and initialization required to open). Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73498 — Andrey Andreev — 2014-04-01T09:20:30+00:00 URL: https://externals.io/message/73338#73498 · In reply to: #73495 Hi, > > […] > > > Yes, it's an API for it. > It's more efficient than user land functions as it requires needless > close and open (and initialization required to open). I'm confused now, does it call open() or does it not? If it doesn't - it's unsafe. If it does - it's unclear, redundant and optimizes by silently discarding already open resources (without properly closing them). Either way, I like efficiency but I also see it as flawed. Cheers, Andrey. --- ## #73594 — Yasuo Ohgaki — 2014-04-04T09:48:43+00:00 URL: https://externals.io/message/73338#73594 · In reply to: #73498 Hi Andrey, > > […] > > I'm confused now, does it call open() or does it not? > If it doesn't - it's unsafe. > It's safe as long as handler locks data during read/write. Example is memcached save handler with unlocked session. If it does - it's unclear, redundant and optimizes by silently > discarding already open resources (without properly closing them). > > Either way, I like efficiency but I also see it as flawed. > Save handler may lock/unlock session data. There is no standardized way for it. Since there would not be lock option for session manager, I don't mind removing it. There is not much point to have it without lock option for session manager. I would like to have `session_gc()` than `session_reset()`. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #73595 — Andrey Andreev — 2014-04-04T10:07:56+00:00 URL: https://externals.io/message/73338#73595 · In reply to: #73594 Hi, > Hi Andrey, > > > > […] > > > It's safe as long as handler locks data during read/write. > Example is memcached save handler with unlocked session. I had something other in mind and you didn't really answer my question, but ... there you have it - it can be unsafe even by your own words. :) > > […] > > > Save handler may lock/unlock session data. There is no standardized > way for it. > > Since there would not be lock option for session manager, I don't mind > removing it. There is not much point to have it without lock option for > session manager. I would like to have `session_gc()` than `session_reset()`. This sounds a bit like you're proposing some kind of a trade agreement. When you put it that way, I'd also rather have `session_gc()` instead of `session_reset()`, but that's not the point. My initial argument was that these features need to be brainstormed here on internals and there was no discussion about them at all. Cheers, Andrey. --- ## #73596 — Yasuo Ohgaki — 2014-04-04T22:31:37+00:00 URL: https://externals.io/message/73338#73596 · In reply to: #73595 Hi Andrey, > > > […] > > I had something other in mind and you didn't really answer my > question, but ... there you have it - it can be unsafe even by your > own words. :) > Regarding to safety of session data management, session management is _not_ safe anyway. Locked session does not guarantee data consistency due to nature of HTTP session management. Locked session provides a litter better consistency. That's all and no more than that. i.e. It's false sense of safety. That said better is better. That's the reason why I'm proposing various _better_ way of management than now. > > […] > > This sounds a bit like you're proposing some kind of a trade > agreement. When you put it that way, I'd also rather have `session_gc()` > instead of `session_reset()`, but that's not the point. My initial > argument was that these features need to be brainstormed here on > internals and there was no discussion about them at all. It's not trade. `session_gc()` is mandatory and I'll add it anyway :) Periodic GC is much better way than probability based GC. There must be API for it. I think there wouldn't be argument for this. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net