launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05157
[Merge] lp:~allenap/launchpad/longpoll-finish-read-only-request into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/longpoll-finish-read-only-request into lp:launchpad with lp:~allenap/launchpad/longpoll-associate-consumer-not-now as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/launchpad/longpoll-finish-read-only-request/+merge/77915
This branch does two things:
- For those publications that have finishReadOnlyRequest() methods,
this branch expands their responsibilities to also fire a newly
defined event, FinishReadOnlyRequestEvent.
- Wires up the two globally defined RabbitSession instances to these
events.
--
https://code.launchpad.net/~allenap/launchpad/longpoll-finish-read-only-request/+merge/77915
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/longpoll-finish-read-only-request into lp:launchpad.
=== modified file 'lib/canonical/launchpad/doc/webapp-publication.txt'
--- lib/canonical/launchpad/doc/webapp-publication.txt 2011-06-28 15:04:29 +0000
+++ lib/canonical/launchpad/doc/webapp-publication.txt 2011-10-03 11:47:27 +0000
@@ -192,7 +192,7 @@
>>> requestfactory, publicationfactory = factory()
>>> publicationfactory
- <class 'canonical.launchpad.webapp.publication.LaunchpadBrowserPublication'>
+ <class '...LaunchpadBrowserPublication'>
If the request comes in on one of the virtual hosts, the request
factory is wrapped in an ApplicationServerSettingRequestFactory that
@@ -200,7 +200,7 @@
host configured settings.
>>> type(requestfactory)
- <class 'canonical.launchpad.webapp.servers.ApplicationServerSettingRequestFactory'>
+ <class '...ApplicationServerSettingRequestFactory'>
>>> request = requestfactory(StringIO(''), environment)
>>> type(request)
<class 'canonical.launchpad.webapp.servers.LaunchpadBrowserRequest'>
@@ -322,7 +322,7 @@
True
>>> requestfactory, publicationfactory = factory()
>>> publicationfactory
- <class 'canonical.launchpad.webapp.publication.LaunchpadBrowserPublication'>
+ <class '...LaunchpadBrowserPublication'>
Zope Publisher integration
@@ -895,7 +895,7 @@
some string in its finishReadOnlyRequest().
>>> class MyPublication(LaunchpadBrowserPublication):
- ... def finishReadOnlyRequest(self, txn):
+ ... def finishReadOnlyRequest(self, request, ob, txn):
... print "booo!"
>>> publication = MyPublication(None)
=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py 2011-08-11 19:42:23 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py 2011-10-03 11:47:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -887,6 +887,24 @@
self.request = request
+class IFinishReadOnlyRequestEvent(Interface):
+ """An event which gets sent when the publication is ended"""
+
+ object = Attribute("The object to which this request pertains.")
+
+ request = Attribute("The active request.")
+
+
+class FinishReadOnlyRequestEvent:
+ """An event which gets sent when the publication is ended"""
+
+ implements(IFinishReadOnlyRequestEvent)
+
+ def __init__(self, ob, request):
+ self.object = ob
+ self.request = request
+
+
class StormRangeFactoryError(Exception):
"""Raised when a Storm result set cannot be used for slicing by a
StormRangeFactory.
=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py 2011-09-30 19:34:28 +0000
+++ lib/canonical/launchpad/webapp/publication.py 2011-10-03 11:47:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -62,6 +62,7 @@
import canonical.launchpad.webapp.adapter as da
from canonical.launchpad.webapp.dbpolicy import LaunchpadDatabasePolicy
from canonical.launchpad.webapp.interfaces import (
+ FinishReadOnlyRequestEvent,
IDatabasePolicy,
ILaunchpadRoot,
INotificationResponse,
@@ -151,7 +152,7 @@
# exception was added as a result of bug 597324 (message #10 in
# particular).
return
- referrer = request.getHeader('referer') # match HTTP spec misspelling
+ referrer = request.getHeader('referer') # Match HTTP spec misspelling.
if not referrer:
raise NoReferrerError('No value for REFERER header')
# XXX: jamesh 2007-04-26 bug=98437:
@@ -531,10 +532,11 @@
# Abort the transaction on a read-only request.
# NOTHING AFTER THIS SHOULD CAUSE A RETRY.
if request.method in ['GET', 'HEAD']:
- self.finishReadOnlyRequest(txn)
+ self.finishReadOnlyRequest(request, ob, txn)
elif txn.isDoomed():
- txn.abort() # Sends an abort to the database, even though
+ # The following sends an abort to the database, even though the
# transaction is still doomed.
+ txn.abort()
else:
txn.commit()
@@ -552,12 +554,13 @@
# calling beforeTraversal or doing proper cleanup.
pass
- def finishReadOnlyRequest(self, txn):
+ def finishReadOnlyRequest(self, request, ob, txn):
"""Hook called at the end of a read-only request.
By default it abort()s the transaction, but subclasses may need to
commit it instead, so they must overwrite this.
"""
+ notify(FinishReadOnlyRequestEvent(ob, request))
txn.abort()
def callTraversalHooks(self, request, ob):
@@ -736,11 +739,11 @@
if IBrowserRequest.providedBy(request):
OpStats.stats['http requests'] += 1
status = request.response.getStatus()
- if status == 404: # Not Found
+ if status == 404: # Not Found
OpStats.stats['404s'] += 1
- elif status == 500: # Unhandled exceptions
+ elif status == 500: # Unhandled exceptions
OpStats.stats['500s'] += 1
- elif status == 503: # Timeouts
+ elif status == 503: # Timeouts
OpStats.stats['503s'] += 1
# Increment counters for status code groups.
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2011-08-31 20:49:46 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2011-10-03 11:47:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=W0231,E1002
@@ -34,6 +34,7 @@
from zope.app.server import wsgi
from zope.app.wsgi import WSGIPublisherApplication
from zope.component import getUtility
+from zope.event import notify
from zope.interface import (
alsoProvides,
implements,
@@ -81,6 +82,7 @@
)
from canonical.launchpad.webapp.errorlog import ErrorReportRequest
from canonical.launchpad.webapp.interfaces import (
+ FinishReadOnlyRequestEvent,
IAPIDocRoot,
IBasicLaunchpadRequest,
IBrowserFormNG,
@@ -1206,8 +1208,9 @@
else:
return super(WebServicePublication, self).getResource(request, ob)
- def finishReadOnlyRequest(self, txn):
+ def finishReadOnlyRequest(self, request, ob, txn):
"""Commit the transaction so that created OAuthNonces are stored."""
+ notify(FinishReadOnlyRequestEvent(ob, request))
# Transaction commits usually need to be aware of the possibility of
# a doomed transaction. We do not expect that this code will
# encounter doomed transactions. If it does, this will need to be
=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
--- lib/canonical/launchpad/webapp/tests/test_servers.py 2011-08-16 19:10:40 +0000
+++ lib/canonical/launchpad/webapp/tests/test_servers.py 2011-10-03 11:47:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E1002
@@ -32,19 +32,24 @@
Interface,
)
+from canonical.launchpad.webapp.interfaces import IFinishReadOnlyRequestEvent
+from canonical.launchpad.webapp.publication import LaunchpadBrowserPublication
from canonical.launchpad.webapp.servers import (
ApplicationServerSettingRequestFactory,
LaunchpadBrowserRequest,
LaunchpadTestRequest,
VHostWebServiceRequestPublicationFactory,
VirtualHostRequestPublicationFactory,
+ web_service_request_to_browser_request,
WebServiceClientRequest,
WebServicePublication,
WebServiceRequestPublicationFactory,
WebServiceTestRequest,
- web_service_request_to_browser_request,
- )
-from lp.testing import TestCase
+ )
+from lp.testing import (
+ EventRecorder,
+ TestCase,
+ )
class SetInWSGIEnvironmentTestCase(TestCase):
@@ -571,6 +576,62 @@
browser_request.get('PATH_INFO'))
+class LoggingTransaction:
+
+ def __init__(self):
+ self.log = []
+
+ def commit(self):
+ self.log.append("COMMIT")
+
+ def abort(self):
+ self.log.append("ABORT")
+
+
+class TestFinishReadOnlyRequest(TestCase):
+ # Publications that have a finishReadOnlyRequest() method are obliged to
+ # fire an IFinishReadOnlyRequestEvent.
+
+ def _test_publication(self, publication, expected_transaction_log):
+ # publication.finishReadOnlyRequest() issues an
+ # IFinishReadOnlyRequestEvent and alters the transaction.
+ fake_request = object()
+ fake_object = object()
+ fake_transaction = LoggingTransaction()
+
+ with EventRecorder() as event_recorder:
+ publication.finishReadOnlyRequest(
+ fake_request, fake_object, fake_transaction)
+
+ self.assertEqual(
+ expected_transaction_log,
+ fake_transaction.log)
+
+ finish_events = [
+ event for event in event_recorder.events
+ if IFinishReadOnlyRequestEvent.providedBy(event)]
+ self.assertEqual(
+ 1, len(finish_events), (
+ "Expected only one IFinishReadOnlyRequestEvent, but "
+ "got: %r" % finish_events))
+
+ [finish_event] = finish_events
+ self.assertIs(fake_request, finish_event.request)
+ self.assertIs(fake_object, finish_event.object)
+
+ def test_WebServicePub_fires_FinishReadOnlyRequestEvent(self):
+ # WebServicePublication.finishReadOnlyRequest() issues an
+ # IFinishReadOnlyRequestEvent and commits the transaction.
+ publication = WebServicePublication(None)
+ self._test_publication(publication, ["COMMIT"])
+
+ def test_LaunchpadBrowserPub_fires_FinishReadOnlyRequestEvent(self):
+ # LaunchpadBrowserPublication.finishReadOnlyRequest() issues an
+ # IFinishReadOnlyRequestEvent and aborts the transaction.
+ publication = LaunchpadBrowserPublication(None)
+ self._test_publication(publication, ["ABORT"])
+
+
def test_suite():
suite = unittest.TestSuite()
suite.addTest(DocTestSuite(
=== modified file 'lib/lp/services/messaging/configure.zcml'
--- lib/lp/services/messaging/configure.zcml 2011-09-23 20:20:14 +0000
+++ lib/lp/services/messaging/configure.zcml 2011-10-03 11:47:27 +0000
@@ -9,4 +9,10 @@
<utility
provides=".interfaces.IMessageSession"
component=".rabbit.unreliable_session" />
+ <subscriber
+ for="canonical.launchpad.webapp.interfaces.IFinishReadOnlyRequestEvent"
+ handler=".rabbit.session_finish_handler" />
+ <subscriber
+ for="canonical.launchpad.webapp.interfaces.IFinishReadOnlyRequestEvent"
+ handler=".rabbit.unreliable_session_finish_handler" />
</configure>
=== modified file 'lib/lp/services/messaging/rabbit.py'
--- lib/lp/services/messaging/rabbit.py 2011-10-03 11:47:27 +0000
+++ lib/lp/services/messaging/rabbit.py 2011-10-03 11:47:27 +0000
@@ -136,6 +136,8 @@
# Per-thread sessions.
session = RabbitSession()
+session_finish_handler = (
+ lambda event: session.finish())
class RabbitUnreliableSession(RabbitSession):
@@ -166,6 +168,8 @@
# Per-thread "unreliable" sessions.
unreliable_session = RabbitUnreliableSession()
+unreliable_session_finish_handler = (
+ lambda event: unreliable_session.finish())
class RabbitMessageBase:
=== modified file 'lib/lp/services/messaging/tests/test_rabbit.py'
--- lib/lp/services/messaging/tests/test_rabbit.py 2011-10-03 11:47:27 +0000
+++ lib/lp/services/messaging/tests/test_rabbit.py 2011-10-03 11:47:27 +0000
@@ -14,7 +14,9 @@
import transaction
from transaction._transaction import Status as TransactionStatus
from zope.component import getUtility
+from zope.event import notify
+from canonical.launchpad.webapp.interfaces import FinishReadOnlyRequestEvent
from canonical.testing.layers import (
LaunchpadFunctionalLayer,
RabbitMQLayer,
@@ -95,12 +97,14 @@
class TestRabbitSession(RabbitTestCase):
+ session_factory = RabbitSession
+
def test_interface(self):
- session = RabbitSession()
+ session = self.session_factory()
self.assertThat(session, Provides(IMessageSession))
def test_connect(self):
- session = RabbitSession()
+ session = self.session_factory()
self.assertFalse(session.is_connected)
connection = session.connect()
self.assertTrue(session.is_connected)
@@ -108,20 +112,20 @@
def test_connect_with_incomplete_configuration(self):
self.pushConfig("rabbitmq", host="none")
- session = RabbitSession()
+ session = self.session_factory()
with ExpectedException(
MessagingUnavailable, "Incomplete configuration"):
session.connect()
def test_disconnect(self):
- session = RabbitSession()
+ session = self.session_factory()
session.connect()
session.disconnect()
self.assertFalse(session.is_connected)
def test_is_connected(self):
# is_connected is False once a connection has been closed.
- session = RabbitSession()
+ session = self.session_factory()
session.connect()
# Close the connection without using disconnect().
session._connection.close()
@@ -129,7 +133,7 @@
def test_defer(self):
task = lambda foo, bar: None
- session = RabbitSession()
+ session = self.session_factory()
session.defer(task, "foo", bar="baz")
self.assertEqual(1, len(session._deferred))
[deferred_task] = session._deferred
@@ -142,7 +146,7 @@
# RabbitSession.flush() runs deferred tasks.
log = []
task = lambda: log.append("task")
- session = RabbitSession()
+ session = self.session_factory()
session.defer(task)
session.connect()
session.flush()
@@ -155,7 +159,7 @@
# deferred tasks.
log = []
task = lambda: log.append("task")
- session = RabbitSession()
+ session = self.session_factory()
session.defer(task)
session.connect()
session.reset()
@@ -168,7 +172,7 @@
# deferred tasks.
log = []
task = lambda: log.append("task")
- session = RabbitSession()
+ session = self.session_factory()
session.defer(task)
session.connect()
session.finish()
@@ -177,27 +181,29 @@
self.assertFalse(session.is_connected)
def test_getProducer(self):
- session = RabbitSession()
+ session = self.session_factory()
producer = session.getProducer("foo")
self.assertIsInstance(producer, RabbitRoutingKey)
self.assertIs(session, producer.session)
self.assertEqual("foo", producer.key)
def test_getConsumer(self):
- session = RabbitSession()
+ session = self.session_factory()
consumer = session.getConsumer("foo")
self.assertIsInstance(consumer, RabbitQueue)
self.assertIs(session, consumer.session)
self.assertEqual("foo", consumer.name)
-class TestRabbitUnreliableSession(RabbitTestCase):
+class TestRabbitUnreliableSession(TestRabbitSession):
+
+ session_factory = RabbitUnreliableSession
def raise_AMQPException(self):
raise amqp.AMQPException(123, "Suffin broke.", "Whut?")
def test_finish_suppresses_AMQPException(self):
- session = RabbitUnreliableSession()
+ session = self.session_factory()
session.defer(self.raise_AMQPException)
session.finish()
# Look, no exceptions!
@@ -206,7 +212,7 @@
raise MessagingException("Arm stuck in combine.")
def test_finish_suppresses_MessagingException(self):
- session = RabbitUnreliableSession()
+ session = self.session_factory()
session.defer(self.raise_MessagingException)
session.finish()
# Look, no exceptions!
@@ -215,7 +221,7 @@
raise IOError("Leg eaten by cow.")
def test_finish_suppresses_IOError(self):
- session = RabbitUnreliableSession()
+ session = self.session_factory()
session.defer(self.raise_IOError)
session.finish()
# Look, no exceptions!
@@ -224,7 +230,7 @@
raise Exception("That hent worked.")
def test_finish_does_not_suppress_other_errors(self):
- session = RabbitUnreliableSession()
+ session = self.session_factory()
session.defer(self.raise_Exception)
self.assertRaises(Exception, session.finish)
@@ -407,3 +413,23 @@
self.assertIs(
global_unreliable_session,
getUtility(IMessageSession))
+
+ def _test_session_finish_read_only_request(self, session):
+ # When a read-only request ends the session is also finished.
+ log = []
+ task = lambda: log.append("task")
+ session.defer(task)
+ session.connect()
+ notify(FinishReadOnlyRequestEvent(None, None))
+ self.assertEqual(["task"], log)
+ self.assertEqual([], list(session._deferred))
+ self.assertFalse(session.is_connected)
+
+ def test_global_session_finish_read_only_request(self):
+ # When a read-only request ends the global_session is finished too.
+ self._test_session_finish_read_only_request(global_session)
+
+ def test_global_unreliable_session_finish_read_only_request(self):
+ # When a read-only request ends the global_unreliable_session is
+ # finished too.
+ self._test_session_finish_read_only_request(global_unreliable_session)
