Page MenuHomePhabricator

Fix flaky subscribers policy rule unit test
ClosedPublic

Authored by epriestley on Dec 11 2016, 8:25 PM.
Tags
None
Referenced Files
F13081511: D17029.id40964.diff
Wed, Apr 24, 8:05 PM
F13080977: D17029.id40964.diff
Wed, Apr 24, 12:19 PM
Unknown Object (File)
Mon, Apr 22, 8:49 AM
Unknown Object (File)
Tue, Apr 9, 4:58 PM
Unknown Object (File)
Mon, Apr 8, 1:43 AM
Unknown Object (File)
Sat, Mar 30, 9:06 PM
Unknown Object (File)
Tue, Mar 26, 7:22 PM
Unknown Object (File)
Tue, Mar 26, 7:22 PM
Subscribers
None
Tokens
"Y So Serious" token, awarded by epriestley.

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix flaky subscribers policy rule unit test.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Dec 11 2016, 8:27 PM

I also grepped for other uses of spl_object_hash() but we only have ~2 and they seem safe (the hashed objects can't be garbage collected at least until we start reusing interpreters).

This revision was automatically updated to reflect the committed changes.

Also this change fills me with shame and I formally disavow it.

...but the commit passed tests, so that's something.

It looks like this might have actually fixed it, I haven't seen any more of these yet.

I haven't seen "epriestley added a revision: Unknown Revision." either, but I'm less confident about that issue being rooted here.