multiple variables pointing to FeatureController get out of sync easily

Fixed in stable r11535 (http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/11535) by a commit, but not testable.

Whats landed is a workaround, not a fix.

On the whole I think perhaps we should have it only in the thread local not in the request, because that interface can be used from any code, not only webapp-specific things.

librarian.txt complains that in its doctests, it needs to manually put this into per_thread.features, having already created a LaunchpadTestRequest. Perhaps we should make sure that LaunchpadTestRequest either fires the 'start_request' event, or manually calls the specific equivalent for features?

The thread local is undesirable in general, because we want
(eventually) to have a single point of control to move the context of
some code from thread to thread, or (more likely) to suspend/resume
it.

The security framework and other parts of Zope require a participation
to work at all; I think getFeatureFlag as a magic convenience helper
should be tied into that same thing (which is a long winded way of
saying 'hanging off the request is appropriate').

Code that needs features without a 'request' context should just
create a scope and controller *itself* - its by far and away the
exceptional case, and given the design:
 - heavy caching
 - user-specific rules

we would run a high risk of surprising behaviour if we tie a
controller to any lifetime other than that of a user participation.

Agree with Robert's analysis.

How does one work around this issue?

Say I want to make a unittest to test a template that has feature-flag hidden content.

class TestFoo(TestCaseWithFactory):
    layer = DatabaseFunctionalLayer
    def setUp(self):
        super(TestFoo, self).setUp()
        self.bug_tracker = self.factory.makeBugTracker()

    def test_view_render(self):
        self.useFixture(FeatureFixture({'foo': 'on'}))
        ...
        view = create_initialized_view(
     self.bug_tracker, name='+index', rootsite='bugs',
            current_request=True)
        self.assertTextMatchesExpressionIgnoreWhitespace(
            "Features: {'foo': 'on'}", view.render())

Instead I get Features: {'foo': None} in the generated html.

I was thinking about this, and I wonder if we would be better off
having just a single featurecontroller object, then having functions
that pass in scopes appropriate to the web request or whatever other
context.

There's not really any good reason (I can see) that the controller
itself needs to be per thread and removing it would at least make it
easier to see what's going on.

--
Martin

@bryce - not sure whats up there, its not this bug though: this bug is
about the multiple names we have for 'the current controller'. You'll
need to dig through the FeatureFixture code and see what its actually
doing, or look at other uses of it.

@Martin, I don't understand what you're suggesting.

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

this might have been fixed by https://code.launchpad.net/~bac/launchpad/lptestrequest/+merge/57484 ?

Martin I'm unsure whether the fix I made addresses this problem or not. I was concerned with fixing the problems like the one described by Bryce and was not considering the deeper issue you and Robert describe.

If you (think you) fixed the thing reported by Bryce, I for one am
happy to consider this closed. I think the more specific bug report
is more important/useful than the general thoughts about how it ought
to be structured.