launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05184
lp:~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510 into lp:launchpad
Michael Hudson-Doyle has proposed merging lp:~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #623199 in Launchpad itself: "scripts do not establish valid zope participations"
https://bugs.launchpad.net/launchpad/+bug/623199
Bug #861510 in Launchpad itself: "XMLRPCTestTransport is not torn down properly"
https://bugs.launchpad.net/launchpad/+bug/861510
For more details, see:
https://code.launchpad.net/~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510/+merge/78355
Hi,
This branch does two things:
1) It creates a pattern for storing stuff in the participation (aka request in an appserver context) rather than thread local variables, and
2) Uses this new pattern to store the flag indicating whether to read the hard_timeout feature flag in this way rather on a thread local.
Part 2) fixes bug 861510, which could have been fixed in other ways, but I've been thinking about a way to do 1) and start fixing bug 623199. I have two and a half failed attempts to store the feature controller itself (only) on the participation, but they all got a bit confusing and I decided to pick an easier target (I wanted to fix _some_ problem in this way, to validate the technique). I quite like the fix -- because the flag defaults to False, I can kill off the half baked attempts we have to clear this flag when it needs to be cleared and can be confident that it can't escape the test isolation (this is one of the reasons I just landed a branch that unconditionally tears down the interaction between tests <wink>).
The core understanding I reached is that in Launchpad, the participation is only ever:
(a) an instance of canonical.launchpad.webapp.interaction.Participation (in scripts, mostly)
(b) an instance of canonical.launchpad.webapp.servers.LaunchpadTestRequest (in tests, clearly -- this is the participation type the lp.testing login helpers create by default)
(c) some subclass of canonical.launchpad.webapp.servers.BasicLaunchpadRequest (in the appserver)
So we only need to add an attribute to instances of these three classes and then we can confidently access it whenever there is a zope interaction set up (whenever we're "logged in" in some sense, although this includes the anonymous "user").
In the thread linked to from bug 623199, I proposed using the annotations dictionary that Launchpad's browser requests already have, but when push came to shove I didn't really see the point and just stuck another attribute on -- if we were to use annotations, we'd have to come up with a constant containing the key and import that around and it just felt unnecessary (also some page templates read the features attribute off the request already, so supporting them was easier this way in my previous branches).
--
https://code.launchpad.net/~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510/+merge/78355
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py 2011-09-22 02:29:01 +0000
+++ lib/canonical/launchpad/webapp/adapter.py 2011-10-06 06:45:33 +0000
@@ -71,6 +71,7 @@
ReadOnlyModeViolation,
SLAVE_FLAVOR,
)
+from canonical.launchpad.webapp.interaction import get_participation_extras
from canonical.launchpad.webapp.opstats import OpStats
from canonical.lazr.timeout import set_default_timeout_function
from lp.services import features
@@ -195,7 +196,6 @@
_local.enable_timeout = enable_timeout
_local.commit_logger = CommitLogger(transaction)
transaction.manager.registerSynch(_local.commit_logger)
- set_permit_timeout_from_features(False)
def clear_request_started():
@@ -289,7 +289,7 @@
:param enabled: If True permit looking up request timeouts in
feature flags.
"""
- _local._permit_feature_timeout = enabled
+ get_participation_extras().permit_timeout_from_features = enabled
def _get_request_timeout(timeout=None):
@@ -302,7 +302,9 @@
return None
if timeout is None:
timeout = config.database.db_statement_timeout
- if getattr(_local, '_permit_feature_timeout', False):
+ participation_extras = get_participation_extras()
+ if (participation_extras is not None
+ and participation_extras.permit_timeout_from_features):
set_permit_timeout_from_features(False)
try:
timeout_str = features.getFeatureFlag('hard_timeout')
=== modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.py'
--- lib/canonical/launchpad/webapp/ftests/test_adapter.py 2010-10-04 20:46:55 +0000
+++ lib/canonical/launchpad/webapp/ftests/test_adapter.py 2011-10-06 06:45:33 +0000
@@ -7,23 +7,7 @@
import unittest
from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
-from canonical.launchpad.webapp import adapter
from canonical.testing.layers import LaunchpadFunctionalLayer
-from lp.testing import TestCase
-
-
-class TestTimeout(TestCase):
-
- def test_set_permit_timeout_from_features(self):
- adapter.set_permit_timeout_from_features(True)
- self.assertTrue(adapter._local._permit_feature_timeout)
- adapter.set_permit_timeout_from_features(False)
- self.assertFalse(adapter._local._permit_feature_timeout)
-
- def test_set_request_started_disables_flag_timeout(self):
- adapter.set_request_started()
- self.addCleanup(adapter.clear_request_started)
- self.assertFalse(adapter._local._permit_feature_timeout)
def test_suite():
=== modified file 'lib/canonical/launchpad/webapp/interaction.py'
--- lib/canonical/launchpad/webapp/interaction.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/interaction.py 2011-10-06 06:45:33 +0000
@@ -47,6 +47,7 @@
from canonical.launchpad.webapp.interfaces import (
IOpenLaunchBag,
+ IParticipationExtras,
IPlacelessAuthUtility,
)
@@ -54,6 +55,7 @@
__all__ = [
'ANONYMOUS',
'get_current_principal',
+ 'get_participation_extras',
'setupInteraction',
'setupInteractionByEmail',
'setupInteractionForPerson',
@@ -166,8 +168,38 @@
class Participation:
- """A very simple participation."""
- implements(IParticipation)
-
+ """Our most simple participation."""
+
+ implements(IParticipation, IParticipationExtras)
+
+ # From IParticipation
interaction = None
principal = None
+
+ # From IParticipationExtras
+ permit_timeout_from_features = False
+
+
+def get_participation_extras():
+ """Return the active provider of `IParticipationExtras`.
+
+ This is looked up from the interaction. If there is no interaction or
+ suitable participation, then return None.
+ """
+ interaction = queryInteraction()
+ if interaction is None:
+ return None
+ # XXX We could perhaps be less LBYL-ish here and assert that there is one
+ # participation and that it provides IParticipationExtras (because in any
+ # expected situation today, that's certainly the case).
+ participations = [
+ participation
+ for participation in interaction.participations
+ if IParticipationExtras.providedBy(participation)
+ ]
+ if not participations:
+ return None
+ assert len(participations) == 1, (
+ "We expect only one provide of IParticipationExtras in the "
+ "interaction. Got %s." % len(participations))
+ return participations[0]
=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py 2011-10-03 11:36:55 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py 2011-10-06 06:45:33 +0000
@@ -310,6 +310,19 @@
'''
+class IParticipationExtras(Interface):
+ """Extra attributes we make all our participations have.
+
+ If you add something here, you should go and edit
+ `canonical.launchpad.webapp.interaction.Participation` and
+ `canonical.launchpad.webapp.servers.BasicLaunchpadRequest`.
+ """
+
+ permit_timeout_from_features = Attribute(
+ "A boolean indicating whether to read the 'hard_timeout' feature "
+ "flag. XXX explain more here.")
+
+
#
# Request
#
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2011-10-03 11:36:55 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2011-10-06 06:45:33 +0000
@@ -90,6 +90,7 @@
ILaunchpadProtocolError,
INotificationRequest,
INotificationResponse,
+ IParticipationExtras,
IPlacelessAuthUtility,
IPlacelessLoginSource,
OAuthPermission,
@@ -556,9 +557,12 @@
class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
- """Mixin request class to provide stepstogo."""
-
- implements(IBasicLaunchpadRequest)
+ """Mixin request class to provide stepstogo and IParticipationExtras."""
+
+ implements(IBasicLaunchpadRequest, IParticipationExtras)
+
+ # IParticipationExtras
+ permit_timeout_from_features = False
def __init__(self, body_instream, environ, response=None):
self.traversed_objects = []
@@ -866,12 +870,17 @@
False
"""
- implements(INotificationRequest, IBasicLaunchpadRequest, IParticipation,
- canonical.launchpad.layers.LaunchpadLayer)
+ implements(
+ INotificationRequest, IBasicLaunchpadRequest, IParticipation,
+ IParticipationExtras, canonical.launchpad.layers.LaunchpadLayer)
+
# These two attributes satisfy IParticipation.
principal = None
interaction = None
+ # And this one satisfies IParticipationExtras.
+ permit_timeout_from_features = False
+
def __init__(self, body_instream=None, environ=None, form=None,
skin=None, outstream=None, method='GET', **kw):
super(LaunchpadTestRequest, self).__init__(
=== added file 'lib/canonical/launchpad/webapp/tests/test_interaction.py'
--- lib/canonical/launchpad/webapp/tests/test_interaction.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_interaction.py 2011-10-06 06:45:33 +0000
@@ -0,0 +1,23 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for interaction helpers."""
+
+__metaclass__ = type
+
+from zope.security.interfaces import IParticipation
+
+from canonical.launchpad.webapp.interaction import Participation
+from canonical.launchpad.webapp.interfaces import IParticipationExtras
+from canonical.testing.layers import BaseLayer
+from lp.testing import TestCase
+
+
+class TestParticipationInterfaces(TestCase):
+ layer = BaseLayer
+
+ def test_Participation_implements_IParticipation(self):
+ self.assertProvides(Participation(), IParticipation)
+
+ def test_Participation_implements_IParticipationExtras(self):
+ self.assertProvides(Participation(), IParticipationExtras)
=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
--- lib/canonical/launchpad/webapp/tests/test_servers.py 2011-10-03 11:36:55 +0000
+++ lib/canonical/launchpad/webapp/tests/test_servers.py 2011-10-06 06:45:33 +0000
@@ -32,7 +32,10 @@
Interface,
)
-from canonical.launchpad.webapp.interfaces import IFinishReadOnlyRequestEvent
+from canonical.launchpad.webapp.interfaces import (
+ IFinishReadOnlyRequestEvent,
+ IParticipationExtras,
+ )
from canonical.launchpad.webapp.publication import LaunchpadBrowserPublication
from canonical.launchpad.webapp.servers import (
ApplicationServerSettingRequestFactory,
@@ -432,6 +435,10 @@
"/sabbra/cadabra?tuesday=gone",
request.getURL(include_query=True, path_only=True))
+ def test_provides_IParticipationExtras(self):
+ request = self.request_factory(StringIO.StringIO(''), {})
+ self.assertProvides(request, IParticipationExtras)
+
class TestLaunchpadBrowserRequestMixinWithLaunchpadBrowserRequest(
TestLaunchpadBrowserRequestMixin, TestCase):
=== renamed file 'lib/lp/bugs/doc/bugtracker-tokens.txt.disabled' => 'lib/lp/bugs/doc/bugtracker-tokens.txt'
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-10-05 21:46:17 +0000
+++ lib/lp/testing/__init__.py 2011-10-06 06:45:33 +0000
@@ -115,7 +115,6 @@
)
from canonical.launchpad.webapp.adapter import (
print_queries,
- set_permit_timeout_from_features,
start_sql_logging,
stop_sql_logging,
)
@@ -579,7 +578,6 @@
self.addCleanup(
self.attachLibrarianLog,
LibrarianLayer.librarian_fixture)
- set_permit_timeout_from_features(False)
@adapter(ErrorReportEvent)
def _recordOops(self, event):
