revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00
Code Issues Releases Wiki Activity

Fix flaky subscribers policy rule unit test

Browse source 
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.

We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.

Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).

If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.

Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.

Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17029
This commit is contained in:
epriestley 2016-12-11 12:22:19 -08:00
parent 42896f9f90
commit 237f94b830
2 changed files with 31 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
src/applications/policy/rule/PhabricatorPolicyRule.php
View file

@ -144,9 +144,30 @@ abstract class PhabricatorPolicyRule extends Phobject {
private static function getObjectPolicyCacheKey(
PhabricatorPolicyInterface $object,
PhabricatorPolicyRule $rule) {
$hash = spl_object_hash($object);
$rule = get_class($rule);
return 'policycache.'.$hash.'.'.$rule;
// NOTE: This is quite a bit of a hack, but we don't currently have a
// better way to carry hints from the TransactionEditor into PolicyRules
// about pending policy changes.
// Put some magic secret unique value on each object so we can pass
// information about it by proxy. This allows us to test if pending
// edits to an object will cause policy violations or not, before allowing
// those edits to go through.
// Some better approaches might be:
// - Use traits to give `PhabricatorPolicyInterface` objects real
// storage (requires PHP 5.4.0).
// - Wrap policy objects in a container with extra storage which the
// policy filter knows how to unbox (lots of work).
// When this eventually gets cleaned up, the corresponding hack in
// LiskDAO->__set() should also be cleaned up.
static $id = 0;
if (!isset($object->_hashKey)) {
@$object->_hashKey = 'object.id('.(++$id).')';
}
return $object->_hashKey;
}

11
src/infrastructure/storage/lisk/LiskDAO.php
View file

@ -1780,10 +1780,13 @@ abstract class LiskDAO extends Phobject {
* @task util
*/
public function __set($name, $value) {
phlog(
pht(
'Wrote to undeclared property %s.',
get_class($this).'::$'.$name));
// Hack for policy system hints, see PhabricatorPolicyRule for notes.
if ($name != '_hashKey') {
phlog(
pht(
'Wrote to undeclared property %s.',
get_class($this).'::$'.$name));
}
$this->$name = $value;
}