Problem/Motivation
Repeatedly calling admin/content where there are 10 items of content listed (for example) causes key_value_expire to grow by 2 rows and 2MB for each request. Apache Bench testing for me caused the table to grow to 1.5GB, and a disk-full incident (on a VM with small virtual disk) & wrecked the db. Since 1000 requests equates to 2GB for this table, if a bot were to have access to a form which also causes storage to balloon with repeated requests, that may cause the db on a live site to become unstable.
Steps to reproduce: new git install of Drupal core. Create an authenticated user with permission to access the Content Overview page. Use devel_generate to make 10 pieces of content. Log in as that authenticated user, copy its cookie, and access the page repeatedly using Apache Bench (a tutorial on passing a session cookie to AB is here http://ezra-g.com/blog/20080229/benchmarking-authenticated-drupal-users-...). Check how much the key_value_expire has grown per page request.
Proposed resolution
Implement Serializable on the ViewExectuable object to decrease the footprint when it's serialized. Implement __sleep methods on ViewUi and View (storage) so the executable object is not serialized with them either (we can just create a new one in those instances pretty easily).
Remaining tasks
Tests
User interface changes
None.
API changes
None.
Beta phase evaluation
Issue category | Bug because Drupal should be able to manage its memory/storage usage in a sane manner. |
---|---|
Issue priority | Critical because runaway $form_state storage in the {key_value} table can create a a DOS situation with some database configurations. |
Prioritized changes | The main goal of this issue is scalability and addressed a critical bug. |
Disruption | None; no API change, no BC breaks. |
Comment | File | Size | Author |
---|---|---|---|
#115 | 2252763-114.patch | 1.44 KB | damiankloip |
#107 | interdiff-2252763-107.txt | 4.48 KB | damiankloip |
#107 | 2252763-107.patch | 24.83 KB | damiankloip |
#105 | interdiff-2252763-105.txt | 720 bytes | damiankloip |
#102 | interdiff-2252763-102.txt | 3.42 KB | damiankloip |
Comments
Comment #1
John_B CreditAttribution: John_B commentedComment #2
John_B CreditAttribution: John_B commentedComment #3
John_B CreditAttribution: John_B commentedComment #4
catchWhich form is causing the data to balloon that much? Is it per-session or every request?
Could you dump the data being cached and attach it as a .txt?
Comment #5
BerdirWe've noticed that before on the node add form, but I'm not sure why it would happen on admin/content, possibly the views form?
We have other, related issues to save less in the key value store.
Comment #6
John_B CreditAttribution: John_B commentedOn repeating the experiment, the page requests to admin/content are consistently producing 2 rows and .35MB per request. So a lot smaller but arguably still and issue. A dump of the table after 10 requests is attached.
Comment #7
tim.plunkettIs #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables directly related or just similar?
Comment #8
Wim Leers#7: that's similar, but this is a bigger problem, because it occurs immediately, not due to keeping things around for too long.
I was able to reproduce this. Create 5 nodes, visit
admin/content
, look atkey_value_expire
's newly addedform_state
row … and that's 225 KiB. With 10 nodes, it's 263 KiB. It looks like things are being serialized that shouldn't be serialized.Comment #9
damiankloip CreditAttribution: damiankloip commentedThis is kind of the same issue? This will only be a problem if things also stay around for too long? There is one issue IMO - table size getting out of control easily.
Comment #10
damiankloip CreditAttribution: damiankloip commentedThis will happen on any form with an #ajax element.
ajax_process_form()
will set$form_state['cache'] = TRUE
. So each request with a new form ID etc... will get cached just from the GET request to retrieve the form.Comment #11
John_B CreditAttribution: John_B commentedThe point is probably obvious to core specialists, but FWIW I checked that repeated identical hits run with this type of test on a D7 webform or ajax webform do not lead to any D7 cache table growing.
Comment #12
damiankloip CreditAttribution: damiankloip commentedYeah, I think this is a D8 specific issue :/
Comment #13
Wim Leers#9: No it's not the same problem. The problem is that each individual cache entry is *enormous*. A single
form_state
cache entry containing >250 KiB of data… that's surely not intentional? AFAICT, the problem is that far too many objects are being serialized in Views exposed filter forms. If we could reduce the number of things being serialized, then the problem should go away! :)Comment #14
catchI think the fact that ajax-enabled forms generate a new cache entry for every page hit is as much of a problem as the cache size. Even if we make the cache size smaller, a sufficient number of hits still runs the risk of filling up the database. As it stands I could set up a browser auto-refresh plugin and take some sites down just with that.
Feels like we ought to be able to reduce this to one entry per-session per-form, or possibly better than that. MIght want to split this into two issues?
Comment #15
damiankloip CreditAttribution: damiankloip commentedI agree with catch, and what I was saying in #10 - This affects any ajax enabled form. So that is definitely not a problem just pertaining to views. Making the cache entries smaller will definitely not make the problem go away, just dilute it slightly.
Created another issue as this seems to be just for the size: #2263569: Bypass form caching by default for forms using #ajax.
Comment #16
catchComment #17
damiankloip CreditAttribution: damiankloip commentedSo, one option is to pass around the view id and display id instead. The downside is we then need to load the view set the display, and init handlers each time. The other option is to look at improving the DependencySerialization implementation as ViewExecutable just extends DependencySerialization currently, which does not serialize much as it is only looking for direct object properties.
Example form state KVE entries attached also for before and after. You can see the difference just from the file sizes. That's about a 98% decrease! I have used the admin/content view with one node.
Comment #18
damiankloip CreditAttribution: damiankloip commentedWith the DependencySerialization approach. Bear in mind these are both rough patches atm!
Comment #23
skipyT CreditAttribution: skipyT commentedrerolled the patch and added the current user also in the wakeup.
Comment #24
skipyT CreditAttribution: skipyT commentedComment #25
damiankloip CreditAttribution: damiankloip commentedWe need the current user to be set though?
Comment #26
damiankloip CreditAttribution: damiankloip commentedSorry, you have added it. That interdiff is very misleading... :)
So I think in an ideal work, we would go with my first approach. I.e. don't serialize views. wdyt?
Comment #28
skipyT CreditAttribution: skipyT commentedRerolled the first patch. Which is taking out the views from the from_state.
Comment #30
skipyT CreditAttribution: skipyT commentedI rerolled yesterday the patch, but it isn't worked. It seems we have many tests failing because the exposed form isn't working anymore. Checking it.
Comment #31
skipyT CreditAttribution: skipyT commentedI modified the code to work with the exposed forms. We still need the sleep function to get out the views from form state. It seems the callback object is storing the views and we need to get rid of that. but we need the callback objects for ajax forms.
Comment #33
skipyT CreditAttribution: skipyT commentedi tried to resolve the fatal errors. for now I have a huge hack for this. but lets see what we get at the tests.
Comment #35
xjmComment #36
larowlanWe need an issue summary update here - specifically the 'proposed resolution'
Comment #37
dawehnerWell, right we need a solution for this issue.
Comment #38
skipyT CreditAttribution: skipyT commentedComment #39
damiankloip CreditAttribution: damiankloip commentedI like the serialzation of the view approach too, this works in the sense that the view takes up hardy any space now, but more data is put in there that is not there when the entire view is cached.
So this will not really work at the moment, but this is an idea.
Comment #40
damiankloip CreditAttribution: damiankloip commentedWith more things added to help keep the state.
Comment #41
dawehnerCould we extract that into a helper method?
Comment #43
damiankloip CreditAttribution: damiankloip commentedok, let's see how much better this does.
I haven't looks at extracting that into a helper method yet, one problem that could arise from this is that in ViewsExposedForm::submitForm the $exclude array is passed to the exposed form plugin first (exposedFormSubmit) method where the $exclude array can be altered... So not sure if we need to use the same logic here too, probably. Otherwise there could be differences. Or we just keep the raw input for now as-is and be done with it? :) Or we add a getExposedRawInput() method that does this for us?
Comment #45
damiankloip CreditAttribution: damiankloip commentedok, so no views forms work, no biggie ;)
Comment #46
damiankloip CreditAttribution: damiankloip commentedOK, so it was an issue with the form handlers expecting the view to be executed already. If it was previously executed, we should re execute it when we unserialize I think?
Comment #48
dawehnerWe certainly should explain why we implement it.
What about setItemsPerPage, setOffset, set AjaxEnabled etc.?
Comment #49
damiankloip CreditAttribution: damiankloip commentedSome more changes.
Comment #50
damiankloip CreditAttribution: damiankloip commentedAnd with the storage back to ID.
Comment #53
martin107 CreditAttribution: martin107 commentedI can see a few flaws which are easy to fix - but before I start to make changes, the patch needs a reroll.
No conflicts just auto merging.
Comment #55
damiankloip CreditAttribution: damiankloip commentedBring back the other tests, and trying out something else with ViewUI. Will still not work.
Comment #56
damiankloip CreditAttribution: damiankloip commentedThis.
Comment #59
dawehnerJust tried to understand what damian was talking about. So what about updating
the storage on the ViewUI object, if needed? Not sure, whether this quick fix is sane here, at least things don't fatal anymore.
Comment #61
damiankloip CreditAttribution: damiankloip commentedmh, not sure. That will still load the storage object again which I think is a problem. If we are using the view UI object I think we only want to store and use that. Something like this maybe:
Comment #63
damiankloip CreditAttribution: damiankloip commentedok let's try this then. Should bring the exceptions down.
Comment #65
damiankloip CreditAttribution: damiankloip commentedReroll after some changes in the ViewUI class. Should be same fails.
Comment #67
martin107 CreditAttribution: martin107 commentedLooking at the failing test FieldUITest, locally I get a smaller set of fails.. Retesting just to see where we stand.
Comment #70
damiankloip CreditAttribution: damiankloip commentedComment #71
catchComment #74
damiankloip CreditAttribution: damiankloip commentedIt doesn't make sense why just those two assertions would fail.. Looking at the test, those assertions don't seem to make sense anyway, so not sure what was broken before? If a node it set published FALSE, and then made sticky, it should not be published then?
Comment #75
dawehnerGreat work!
They indeed doesn't make sense anymore at all. Its odd that they did not fail but after #2172017: Bulk operations does not respect entity access its just the logic step to remove this stuff.
Its just ridiculous that PHP doesn't have a method for that.
Haha, we excluded the empty string, this is brutal.
Can you quickly explain why this helped so much?
There was afaik a reason why we implemented __sleep and not the \Serializable interface
It would be great if we document quickly why we have to do that.
+1
Comment #76
damiankloip CreditAttribution: damiankloip commentedThanks Daniel.
1. Yes, utterly ridiculous, I want one of those. Maybe Drupal should have a helper?
2. Yes, I know. wat?!?
3. We want to clean up the form state, this was just here out of convenience. Also, if you are creating a new exectuable things get a little bit crazy/broken with old references
4. I went with Serialzable on the ViewExectuable only so I could return the values I wanted to (pretty much for storage ID)
5. Remove that, now it is just always the ID only.
6. :) much better
Comment #77
damiankloip CreditAttribution: damiankloip commentedWill do some tests shortly. Should be quite easy.
Comment #78
dawehnerLovely!
Comment #79
dawehnerUps, tests are missing. Well, you could argue that we have quite some implicit test coverage.
Comment #80
jhedstromIs this needed? I don't see it used anywhere, and it would also create a dependency on the views_ui module?
Comment #81
damiankloip CreditAttribution: damiankloip commented...create a dependency on the views_ui module? - HA :)
But yes, we don't need it anymore, thanks! I will roll that into the next patch.
Comment #82
Fabianx CreditAttribution: Fabianx commentedCNW per #80
Comment #83
larowlanTagging as a task for DrupalSouth sprint
Comment #84
dashaforbes CreditAttribution: dashaforbes commentedRemoved use statements from ViewExecutable.php
Comment #85
damiankloip CreditAttribution: damiankloip commentedThis is needs work for test, that I am working on. Removing all of those use statements is out of scope here. Sorry!
Comment #86
damiankloip CreditAttribution: damiankloip commented.
Comment #87
damiankloip CreditAttribution: damiankloip commentedHere are some quick tests, as dawehner already mentioned, we have implicit coverage everywhere for this. Look at all the fails that have been fixed over the (many) previous patch iterations :)
With a quick test:
$form_state before: 175789 bytes
$form_state after: 18154 bytes
Not bad!
Comment #88
dawehnerSo we test serialize -> unserialize but do you think testing the serialized content for itself is something we should try out? So for example
$this->assertFalse(strpos($serialized, '"executable"') !== FALSE)
?Comment #89
damiankloip CreditAttribution: damiankloip commentedMehhh. I am not sure that is worth it. We know the object will get serialized. or do you mean check there is not a executable property when serialized in another object? Either way, I would be inclined to say maybe not worth it?
I could add a similar test for the viewUI object maybe?
Comment #90
dawehnerWell, for now nothing ensures that the bug of this issue is actually fixed right? There is no test covering the fact that we don't waste all that memory.
Comment #91
damiankloip CreditAttribution: damiankloip commentedOK, so we talked about this, we are serializing the actual executable, so let's check the ViewStorage is not in the serialized output.
Comment #92
damiankloip CreditAttribution: damiankloip commentedAnd a quick test for the ViewUI object.
Comment #93
dawehnerIt looks great now! Factor 10 is great!
Comment #94
swentel CreditAttribution: swentel commentedExtreme nitpicks.
if => of
clas => class
Comment #95
martin107 CreditAttribution: martin107 commentedFixed.
Comment #96
mpdonadioBack to RTBC.
Added Beta Eval and a related issue. Because of the DOS possibility, this could potentially also get the security tag.
Comment #97
Fabianx CreditAttribution: Fabianx commentedRTBC + 1, Great work!
Comment #98
damiankloip CreditAttribution: damiankloip commentedSpoke to Alex on IRC. Let's just remove the setPublished() calls we do not need anymore, and amend the comment.
re-RTBC please :)
Comment #99
Fabianx CreditAttribution: Fabianx commentedThe test change makes sense.
Comment #100
damiankloip CreditAttribution: damiankloip commentedOops, we can remove the save() calls too. Leaving as is.
Comment #101
alexpottWe can completely remove $executable from the constructor. Talked about this with @damiankloip in IRC.
Comment #102
damiankloip CreditAttribution: damiankloip commentedOk, let's try this. Only using the storage executable. If you need to set it, set it on the storage object first.
Comment #103
alexpott#100 10 files changed, 174 insertions, 54 deletions.
vs
#102 10 files changed, 163 insertions, 64 deletions
Nice. Less code to maintain :) and no chance of ViewUI having an executable out of sync with its storage.
Comment #105
damiankloip CreditAttribution: damiankloip commentedComment #107
damiankloip CreditAttribution: damiankloip commentedDrupal alter wanting to pass the object by reference and creating warnings. Let's use an $executable variable. We also might as well replace the many many calls in the same method with the variable!
Comment #108
Fabianx CreditAttribution: Fabianx commentedNice :)
Is there anything more needed or can this go back to a final review for RTBC?
Comment #109
dawehnerThe changes made by @damiankloip are looking great!
Comment #110
tim.plunkettClassic.
RTBC +1
Comment #111
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d1c0322 and pushed to 8.0.x. Thanks!
Comment #113
Fabianx CreditAttribution: Fabianx commentedstray $this->getExecutable() for follow-up ...
Comment #114
Wim LeersGreat work!
Comment #115
damiankloip CreditAttribution: damiankloip commentedThanks all! Here is that small follow up,there were a couple actually...whoops.
Comment #116
webchickLet's get a follow-up issue for those, so they can do a proper testbot run, etc.
Comment #117
martin107 CreditAttribution: martin107 commentedopened up #2449079: ViewsUI executable cleanup
test bot is on the case.