In the typical creation of an order, we do:
During the save process, the order total gets computed, and we load from the database, even if we just saved them. I suggest that we should store the saved entity in the cache instead of simply clearing it.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 1363814-entity-controller-store-saved-static-cache_12.patch | 7.67 KB | amateescu |
| #12 | interdiff.txt | 6.08 KB | amateescu |
| #9 | 1363814-entity-controller-store-saved-static-cache.patch | 3.01 KB | damien tournoud |
| #6 | locking.patch | 2.64 KB | bojanz |
| #2 | 1363814-entity-controller-store-saved-static-cache.patch | 906 bytes | damien tournoud |
Comments
Comment #1
damien tournoud commentedTagging.
Comment #2
damien tournoud commentedComment #4
bojanz commented#2: 1363814-entity-controller-store-saved-static-cache.patch queued for re-testing.
Comment #6
bojanz commentedSo, this goes against the pessimistic locking, since entityCache never gets emptied, the transaction never gets committed, and bad stuff happens.
There are two ways to solve this:
1) Only doing the performance optimization if pessimistic locking is disabled.
This is okay for our use case (where pessimistic locking might be disabled anyway), but it's still less than ideal.
2) The attached patch.
The problem with this patch is that it does exactly what we needed it to do.
Which means this:
a. You load an order. It gets locked.
b. You save an order. It gets unlocked but stays in entity cache.
c. You load an order again. It is now in an unlocked state, since it was fetched from entity cache.
This means that the pessimistic locking is no longer guaranteed.
Leaving the final call to Damien.
Comment #7
damien tournoud commentedLet's see if this fixes the testing issue.
Comment #9
damien tournoud commentedRerolled.
Comment #11
bojanz commentedStill fails, it seems.
Comment #12
amateescu commented@bojanz forgot to empty the lockedEntities array if no arguments were passed to
resetCache(). This should fix the tests :)New patch and interdiff from #9.
Comment #13
damien tournoud commentedWoo. Great catch, @amateescu!
Comment #14
rszrama commentedCommitted with some minor whitespace changes. I don't see any direct answer to Bojan's question in #6 above with respect to orders "loaded" after a save being unlocked since they came from the cache. Based on Damien's RTBC, I assume this isn't a concern to him.
Thanks again to everyone for the group effort. Tests passed locally, too.
Comment #15
damien tournoud commented@rszrama: this patch doesn't change the behavior in this regard. We should deal with the remaining locking issues in a separate issue.