Comment 10 for bug 631884

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 631884] Re: multiple variables pointing to FeatureController get out of sync easily

I think the pragmatic thing for Bryce is to see if he can just land
his feature with it unconditionally turned on, or perhaps defaulting
to on, thereby avoiding the problems he's having in testing. I think
the value of being able to switch it dynamically is probably
outweighed by the waste of having it stalled for so long. My
apologies again.

Let me try to explain more, though I'm not totally sure this would
work better until I try it.

At the moment calling getFeatureFlag looks at a per-thread variable to
get a FeatureController. The controller holds a reference to the rule
source, a scope check callback, and some caches. The default rule
source talks through Storm to the database.

Arguable problems with this approach:

1- Some test code creates a new FeatureController and assigns it to
the per-thread variable. This may cause trouble if the test actually
ends up reaching code in a different thread, which won't see the
different values. (I don't know if that happens.)

2- The controller is held in both a per-thread variable and a
per-request variable; the repetition is just asking for something to
go wrong.

3- I originally suspected there was a problem with different feature
controllers seeing different database transactions, and that this
accounted for rules defined in one place not being seen in another.
I'm not sure how that could really be true though, because other tests
do modify the database from the test setup code and then look at it
from webapp code, and they see consistent results.

I don't actually know precisely what's going wrong in this situation
so the above are kind of shots in the dark as far as specifically
fixing it, though they do seem like reasonable cleanups in their own
right.

Part of this is because I was trying to allow for features to be
stored, at least in testing, in a non-storm object.

It seems like what would be most consistent with other Launchpad code
and tests is:

* have just one rule source (defined in a global) that points in to storm
* for testing what happens when particular rules are in effect, modify
the rules in the database and per the previous step they will be seen
everywhere
* have a scope source per-thread that is configured at the start of a
request appropriately for that request