Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- Make every setter method call garbageCollection. Originally I think (or hope...) it was so. This makes it unnecessary to call it from cron.
- Make garbageCollection protected. If every setter calls it then testing it can happen via a set().
Comment | File | Size | Author |
---|---|---|---|
#16 | kvstore-1805444-16.patch | 29.48 KB | xjm |
#16 | interdiff15-16.txt | 17.79 KB | xjm |
#15 | kvstore-1805444-15.patch | 12.86 KB | xjm |
#15 | interdiff-12-15.txt | 5.82 KB | xjm |
#12 | kvstore-1805444-12.patch | 7.05 KB | xjm |
Comments
Comment #1
xjmAt a minimum I think we should set a static to make sure it only happens once per request or something. It seemed like an excessive number of queries to me.
Also, I think it should be static; it already doesn't pay attention to what collection it's cleaning up.
#1805412: TempStore: Enable garbage collection on all temp store objects via hook_cron() or similar was the other issue.
Comment #2
xjmLooking at a compromise that won't run N queries, at least. :)
Comment #3
xjmHere's a start. I think a static boolean is a decent tradeoff for not running the same query N times.
garbageCollection()
is still public. Making it protected would mean that we need to add another way to force expirations in order to test it, since the static gets set in the earlier tests. I don't want to usedrupal_static()
because it's not a cache and the world won't end if records don't get deleted until the next request. Dunno.Comment #4
Crell CreditAttribution: Crell commentedStatics in a method are the wrong choice 99.4% of the time, because they're still global and shared across all instances of the class. (That may not make a difference here, for now, but it will bite you later. We ran into that with the DB layer early on.) If you really want it shared between all instances, make it a static class property. If not (and you rarely do), object property.
That said, I'm unclear why this needs to happen on set() calls. Why not on __destruct(), so that it happens at the end of a request, after it's done blocking the request itself? On __destruct(), run call $this->garbageCollect().
You don't need to make it public to test, either. What you're testing is not that calling garbageCollect() does something. You're testing that on destruct, the database state changes. So you create a store object, set something on it that should expire, unset the object, then query the DB table to see if the record is there. That's unit-test safe.
Comment #5
xjm@Crell and @chx and I talked through this a bit in IRC; the consensus is:
$needsGarbageCollection
boolean object property.__destruct()
and rungarbageCollection()
if it's set.Comment #6
xjmLike this.
Comment #8
xjmHm:
It works fine locally. I've requeued again and I'll try it with
run-tests.sh
.Comment #9
xjmRuns fine locally with
run-tests.sh
. Hrm. Waiting on the bot again then. It went to #664 three times in a row, but now it's going to #958, so we'll know shortly if it's just a testbot problem.Comment #10
xjmOkay, I'm stumped. It works fine locally, and everything else is exactly the same as in the other k/v tests and the previous garbage collection tests, which work fine on the bot.
Comment #11
xjmMaybe?
Comment #12
xjmOh, no, it's choking in
DatabaseStorageExpirableTest
. Interesting. So...Comment #14
xjmSo #11 shows the same error as above, and #12 fixes it. This has interesting implications -- do we need to worry in general about the storage going away before the object is destroyed, or is it safe to assume that issue will always be unique to the SimpleTest environment, and change the test so that the stores are no longer referenced on the test class so that they get destroyed before
DatabaseStorageExpirableTest::tearDown()
?The fatal #12 is:
I'll do some more debugging.
Comment #15
xjmOkay, so this solves the issue for
TempStoreDatabaseTest
locally.Still need to restructure the k/v tests and remove the hack for
DatabaseStorageExpirableTest
.Comment #16
xjmAnd fixing
DatabaseStorageExpirableTest
with the minimum possible alteration toStorageTestBase
. Maybe referencing the child test in comments on the parent is a bit goofy, but the alternative would be to completely refactor the k/v tests.Comment #17
chx CreditAttribution: chx commentedLooks a very nice approach to me, well documented and tested.
Comment #18
tim.plunkett#16: kvstore-1805444-16.patch queued for re-testing.
Comment #19
webchickWas not introduced here, but I would love a follow-up to either change this upper-bound value to something sensical or else make it a constant so it's clear what it's referring to (I think I over-read on IRC something about speed of light :))
Down below you are using a simple 5000000 as the upper-bound and this causes no head-scratching. I would recommend doing the same here (and other places where this number is used).
I was confused why this was removed, and tim said:
webchick: its a test helper only that wasn't helpful
webchick: the stores can't be per class, they need to be per method
webchick: or they don't get torn down properly
so this is fair enough.
Committed and pushed to 8.x. Thanks!
Comment #20
xjmFiled: #1808544: Use a more easily recognized arbitrary integer in k/v expiration tests :)
Comment #21
sudhanshushekhar CreditAttribution: sudhanshushekhar commentedFatal error: Exception thrown without a stack frame in Unknown on line 0
I am getting the above error on loading the site. How to resolve this ? Please help
Comment #22
sudhanshushekhar CreditAttribution: sudhanshushekhar commentedRefer following link I resolved my #21 issue from this blog post. Thanks Zufelt.
http://zufelt.ca/blog/trying-catch-missing-stack-frame
Comment #23
xjm