Page MenuHomePhabricator

Fix flaky subscribers policy rule unit test
ClosedPublic

Authored by epriestley on Dec 11 2016, 8:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 30, 1:09 AM
Unknown Object (File)
Wed, Apr 24, 8:05 PM
Unknown Object (File)
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)
Mar 30 2024, 9:06 PM
Unknown Object (File)
Mar 26 2024, 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
Branch
hack1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 14852
Build 19438: Run Core Tests
Build 19437: arc lint + arc unit

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.