Krinkle has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/363831 )
Change subject: MultiWriteBagOStuff: Fix async writes of mutable objects
......................................................................
MultiWriteBagOStuff: Fix async writes of mutable objects
If someone writes an object into a BagOStuff, they typically expect that
later changes to the object will not affect the value stored.
MultiWriteBagOStuff's async write handling was violating this
expectation, which is potentially causing T168040.
Bug: T168040
Change-Id: Ie897b900befdc8998614af06f9339cd07665703e
---
M includes/libs/objectcache/MultiWriteBagOStuff.php
M tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php
2 files changed, 13 insertions(+), 3 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, but someone else must approve
Krinkle: Looks good to me, approved
MaxSem: Looks good to me, but someone else must approve
jenkins-bot: Verified
Objections:
Chad: There's a problem with this change, please improve
diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php
b/includes/libs/objectcache/MultiWriteBagOStuff.php
index 687c67c..d94578d 100644
--- a/includes/libs/objectcache/MultiWriteBagOStuff.php
+++ b/includes/libs/objectcache/MultiWriteBagOStuff.php
@@ -181,6 +181,12 @@
$ret = true;
$args = array_slice( func_get_args(), 3 );
+ if ( $count > 1 && $asyncWrites ) {
+ // Deep-clone $args to prevent misbehavior when
something writes an
+ // object to the BagOStuff then modifies it afterwards,
e.g. T168040.
+ $args = unserialize( serialize( $args ) );
+ }
+
foreach ( $this->caches as $i => $cache ) {
if ( $i >= $count ) {
break; // ignore the lower tiers
diff --git
a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php
b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php
index 775709f..4a9f6cc 100644
--- a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php
@@ -81,22 +81,26 @@
*/
public function testSetDelayed() {
$key = wfRandomString();
- $value = wfRandomString();
+ $value = (object)[ 'v' => wfRandomString() ];
+ $expectValue = clone $value;
// XXX: DeferredUpdates bound to transactions in CLI mode
$dbw = wfGetDB( DB_MASTER );
$dbw->begin();
$this->cache->set( $key, $value );
+ // Test that later changes to $value don't affect the saved
value (e.g. T168040)
+ $value->v = 'bogus';
+
// Set in tier 1
- $this->assertEquals( $value, $this->cache1->get( $key ),
'Written to tier 1' );
+ $this->assertEquals( $expectValue, $this->cache1->get( $key ),
'Written to tier 1' );
// Not yet set in tier 2
$this->assertEquals( false, $this->cache2->get( $key ), 'Not
written to tier 2' );
$dbw->commit();
// Set in tier 2
- $this->assertEquals( $value, $this->cache2->get( $key ),
'Written to tier 2' );
+ $this->assertEquals( $expectValue, $this->cache2->get( $key ),
'Written to tier 2' );
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/363831
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie897b900befdc8998614af06f9339cd07665703e
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Anomie <[email protected]>
Gerrit-Reviewer: Chad <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Legoktm <[email protected]>
Gerrit-Reviewer: MaxSem <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
