Major Feature #26175
Move settings out from session
| Status: | Rejected | Start date: | 2011-04-21 | ||
|---|---|---|---|---|---|
| Priority: | Should have | Due date: | |||
| Assignee: | Reinhard Führicht | % Done: | 40% |
||
| Category: | Code and Architecture | ||||
| Target version: | Stable v1.0 | ||||
| Votes: | 0 |
Description
Actually we have spoken already about that... So basically current process looks so: form config is places into session. So if I have form with big config - each time when form is generated session is filled by settings data (not only by them, but this data tooks major place) with new random key, again and again. So just browsing the site with many form causes memory garbaging. If config is big enough (as in my case) there would be enough ~20 page views to have "memory exhausted" fatal error. My memory limit is 256Mb (yeah, to big!) but it is out quickly as you see.
Just moving settings away from session willn't fix this memory leaks. At this moment i can't tell what will fix them. So can you at least tell me - why do you store settings in session? Maybe this will suggest me how to fix this problem.
History
Updated by Reinhard Führicht about 2 years ago
- Status changed from New to Accepted
- Assignee set to Reinhard Führicht
- Target version set to Stable v1.0
The main reason, why the settings are stored in the session is that they are needed for the AJAX calls. It would be possible to parse the settings in the AJAX scripts again, but information about evaluated conditions would be lost.
The current flow is like this:
- Parse settings
- Find out the current step
- Merge setting with specific settings for the current step
- Parse conditions (and maybe merge the settings again)
- Run classes (PreProcessors)
- Parse conditions as $this->gp values may have changed (and maybe merge the settings again)
- Run classes (InitInterceptors)
- Parse conditions as $this->gp values may have changed (and maybe merge the settings again)
I admit, that it maybe was a bad idea to store the whole settings array in the session, but it is difficult to figure out how to do it right.
Maybe, the following would do the trick:
- Store anything that is in the session right now to a temporary file (typo3temp/formhandler/[randomID].tmp)
- Just store the random ID (and a creation timestamp?) in the session.
- Introduce some kind of garbage collection which removes old temporary files and old entries in the session from time to time.
What do you think?
Updated by Reinhard Führicht almost 2 years ago
- File 26175.patch added
- Status changed from Accepted to Needs Feedback
- % Done changed from 0 to 40
Please have a look at my first try:
- A new class Tx_Formhandler_Session_File
- Stores the session in a temp file
- Temp files get cleared by some kind of garbage collection every time the form gets called.
At first, the files where each about 22KB. I found out that ALL predefined forms settings are saved allthough they are not needed for a single form.
The attached patch clears all "predef." settings after the right predefined form was set. The result: The files are now only 3.7KB.
This is NOT a final patch. It needs some more tweaking (e.g. Make threshold time configurable; rethink storing all session data into the file because it then may contain submitted values too!).
Before I go on, it would be really cool, if you could let me know what you think about it.
Updated by Reinhard Führicht almost 2 years ago
- Status changed from Needs Feedback to Rejected
I committed the mentioned change to only store the current form in session and not all predefined forms.
However, I don't think that storing the session data in a file is necessary nor useful. Formhandler is currently reasonably fast. If we need more speed, we should tweak other parts.
Issue rejected.
Updated by Myroslav Holyak over 1 year ago
Such decision is very sad for me. With big enough config this great extension eats all available memory very fast. I would think about some kind of optional garbage collector, which will work over typo3 scheduler extension. Currently I can't say what will be reason to test each session form entry for actuality, because i just don't know.
Updated by Reinhard Führicht over 1 year ago
Hi Myroslav,
I was only rejecting the "Move settings out of the session and store it into files" part.
Of course, I am still looking for other solutions to make Formhandler "eat" less memory.
Currently, Formhandler needs the settings to be stored in the session. The only thing I can think of right now is the following:
- Store a timestamp of generation in each session
- Each time (or randomly) a new session is generated, Formhandler can loop through the existing sessions and remove the old ones.
It is also possible, that the generation of a new session itself is buggy, but I can't say that for sure right now.
Currently, I am pretty busy, but I promise to look into that in the next weeks. I will open a new issue for that.
Updated by Myroslav Holyak over 1 year ago
- File 26175.patch added
Ok, lets try to use session objects constructor than. Here is my patch for both typo3 and php session abstractions. It just checks for old session instances of current form and removes them. On quick tests result seems to be only one session for each form, which is good.
A problem is - i haven't found any unique identification of form, so I decide to use formValuesPrefix for that. Such decision blocks possibility to have two form with the same formValuesPrefix on the same page. Furthermore - it's disallows globally to have several forms with identical formValuesPrefix. So according to docs you strongly advise to use own "namespace" for each form, it seems that we should require that and check uniqueness at runtime.
By the way - what for protected fields in Tx_Formhandler_Session_PHP? They are inherited from Tx_Formhandler_AbstractClass as I see.
Updated by Reinhard Führicht over 1 year ago
Hi Myroslav,
thanks for your patch.
As I said before, I opened a new issue for that. I attached a new patch there based on your patch.
Have a look here: #29162