Merge lp:~sakuag333/mailman/BUG_non_queue_runner into lp:mailman

[Sandesh Kumar Agrawal]

[Barry Warsaw (barry)]

Hi Sandesh,

Thanks for taking another run at this bug. In general, I think you've
addressed the bug nicely, although there are a few things that still need
fixing. Since the branch is close enough, I'll just fix those when I merge
your branch, but I'll comment on the issues below for future reference.

These are all minor issues. Thanks for your contribution to Mailman!

I'll omit the diff blocks that look fine.

On Jan 17, 2013, at 07:45 AM, Sandesh Kumar Agrawal wrote:

=== modified file 'src/mailman/core/runner.py'
--- src/mailman/core/runner.py 2013-01-01 14:05:42 +0000
+++ src/mailman/core/runner.py 2013-01-17 07:44:42 +0000
> @@ -52,6 +52,7 @@
> @implementer(IRunner)
> class Runner:
> intercept_signals = True
> + is_non_queue_runner = False

In general, I prefer positive flags rather than negative flags. This is
because it's usually more difficult to reason about double negatives. I'll
change the sense of this to "is_queue_runner = True" and then override the
flag to False in the lmtp and rest runners.

>
> def __init__(self, name, slice=None):
> """Create a runner.
> @@ -66,10 +67,15 @@
> section = getattr(config, 'runner.' + name)
> substitutions = config.paths
> substitutions['name'] = name
> - self.queue_directory = expand(section.path, substitutions)
> numslices = int(section.instances)
> - self.switchboard = Switchboard(
> - name, self.queue_directory, slice, numslices, True)
> + # Check whether the runner is queue runner or not; non queue runner should not have queue_directory or switchboard instance.

PEP 8 has a line length limit of 79 characters, so this line needs to be
wrapped.

> + if self.is_non_queue_runner:
> + self.queue_directory = None
> + self.switchboard= None

PEP 8 requires a single space both before and after the = sign.

> + else:
> + self.queue_directory = expand(section.path, substitutions)
> + self.switchboard = Switchboard(
> + name, self.queue_directory, slice, numslices, True)
> self.sleep_time = as_timedelta(section.sleep_time)
> # sleep_time is a timedelta; turn it into a float for time.sleep().
> self.sleep_float = (86400 * self.sleep_time.days +

=== modified file 'src/mailman/core/switchboard.py'
--- src/mailman/core/switchboard.py 2013-01-01 14:05:42 +0000
+++ src/mailman/core/switchboard.py 2013-01-17 07:44:42 +0000
> @@ -90,9 +90,10 @@
> 'Not a power of 2: {0}'.format(numslices))
> self.name = name
> self.queue_directory = queue_directory
> + self.non_queue_runner={'lmtp','rest'}

This should no longer be necessary, right? (I hope it's not! :)

> # If configured to, create the directory if it doesn't yet exist.
> if config.create_paths:
> - makedirs(self.queue_directory, 0770)
> + makedirs(self.queue_directory,0770)

This line should not be changed, and besides, PEP 8 requires a space after the
comma.

> # Fast track for no slices
> self._lower = None
> self._upper = None
> @@ -143,11 +144,11 @@
> ...

=== modified file 'src/mailman/config/mailman.cfg'
--- src/mailman/config/mailman.cfg	2013-01-01 14:05:42 +0000
+++ src/mailman/config/mailman.cfg	2013-01-17 07:44:42 +0000
@@ -60,6 +60,7 @@
 
 [runner.lmtp]
 class: mailman.runners.lmtp.LMTPRunner
+path:
 
 [runner.nntp]
 class: mailman.runners.nntp.NNTPRunner
@@ -72,6 +73,7 @@
 
 [runner.rest]
 class: mailman.runners.rest.RESTRunner
+path:
 
 [runner.retry]
 class: mailman.runners.retry.RetryRunner
 
=== modified file 'src/mailman/core/runner.py'
--- src/mailman/core/runner.py	2013-01-01 14:05:42 +0000
+++ src/mailman/core/runner.py	2013-01-17 07:44:42 +0000
@@ -52,6 +52,7 @@
 @implementer(IRunner)
 class Runner:
     intercept_signals = True
+    is_non_queue_runner = False
 
     def __init__(self, name, slice=None):
         """Create a runner.
@@ -66,10 +67,15 @@
         section = getattr(config, 'runner.' + name)
         substitutions = config.paths
         substitutions['name'] = name
-        self.queue_directory = expand(section.path, substitutions)
         numslices = int(section.instances)
-        self.switchboard = Switchboard(
-            name, self.queue_directory, slice, numslices, True)
+        # Check whether the runner is queue runner or not; non queue runner should not have queue_directory or switchboard instance.
+        if self.is_non_queue_runner:
+            self.queue_directory = None
+            self.switchboard= None
+        else:
+            self.queue_directory = expand(section.path, substitutions)
+            self.switchboard = Switchboard(
+                name, self.queue_directory, slice, numslices, True)
         self.sleep_time = as_timedelta(section.sleep_time)
         # sleep_time is a timedelta; turn it into a float for time.sleep().
         self.sleep_float = (86400 * self.sleep_time.days +
 
=== modified file 'src/mailman/core/switchboard.py'
--- src/mailman/core/switchboard.py	2013-01-01 14:05:42 +0000
+++ src/mailman/core/switchboard.py	2013-01-17 07:44:42 +0000
@@ -90,9 +90,10 @@
             'Not a power of 2: {0}'.format(numslices))
         self.name = name
         self.queue_directory = queue_directory
+        self.non_queue_runner={'lmtp','rest'}
         # If configured to, create the directory if it doesn't yet exist.
         if config.create_paths:
-            makedirs(self.queue_directory, 0770)
+            makedirs(self.queue_directory,0770)
         # Fast track for no slices
         self._lower = None
         self._upper = None
@@ -143,11 +144,11 @@
         data['_parsemsg'] = (protocol == 0)
         # Write to the pickle file the message object and metadata.
         with open(tmpfile, 'w') as fp:
-            fp.write(msgsave)
-            cPickle.dump(data, fp, protocol)
-            fp.flush()
-            os.fsync(fp.fileno())
-        os.rename(tmpfile, filename)
+          	fp.write(msgsave)
+            	cPickle.dump(data, fp, protocol)
+            	fp.flush()
+            	os.fsync(fp.fileno())
+       	os.rename(tmpfile, filename)
         return filebase
 
     def dequeue(self, filebase):
@@ -266,7 +267,9 @@
         name = conf.name.split('.')[-1]
         assert name not in config.switchboards, (
             'Duplicate runner name: {0}'.format(name))
-        substitutions = config.paths
-        substitutions['name'] = name
-        path = expand(conf.path, substitutions)
-        config.switchboards[name] = Switchboard(name, path)
+        # Path is empty for non queue runners. hence check for path and create switchboard instance only for queue runner.
+        if conf.path:
+            substitutions = config.paths
+            substitutions['name'] = name
+            path = expand(conf.path, substitutions)
+            config.switchboards[name] = Switchboard(name, path)
 
=== modified file 'src/mailman/interfaces/runner.py'
--- src/mailman/interfaces/runner.py	2013-01-01 14:05:42 +0000
+++ src/mailman/interfaces/runner.py	2013-01-17 07:44:42 +0000
@@ -50,6 +50,10 @@
 
     def stop():
         """Stop the runner on the next iteration through the loop."""
+      
+    is_non_queue_runner = Attribute("""\
+        A boolean variable to keep track of whether the runner is a queue runner or not.
+        """)
 
     queue_directory = Attribute(
         'The queue directory.  Overridden in subclasses.')
 
=== modified file 'src/mailman/rest/tests/test_root.py'
--- src/mailman/rest/tests/test_root.py	2013-01-01 14:05:42 +0000
+++ src/mailman/rest/tests/test_root.py	2013-01-17 07:44:42 +0000
@@ -25,9 +25,11 @@
 
 
 import unittest
+import os
 
 from urllib2 import HTTPError
 
+from mailman.config import config
 from mailman.testing.helpers import call_api
 from mailman.testing.layers import RESTLayer
 
@@ -67,3 +69,8 @@
                 'receive_own_postings': True,
                 }, method='PUT')
         self.assertEqual(cm.exception.code, 405)
+        
+    def test_rest_queue_directory(self):
+        # Rest is a non queue runner, so it should not have a directory in var/queue
+        is_directory = os.path.isdir(os.path.join(config.paths['QUEUE_DIR'],'rest'))     
+        self.assertEqual(is_directory,False)    
 
=== modified file 'src/mailman/runners/lmtp.py'
--- src/mailman/runners/lmtp.py	2013-01-01 14:05:42 +0000
+++ src/mailman/runners/lmtp.py	2013-01-17 07:44:42 +0000
@@ -49,6 +49,7 @@
 
 from email.utils import parseaddr
 from zope.component import getUtility
+from mailman.interfaces.messages import IMessageStore
 
 from mailman.config import config
 from mailman.core.runner import Runner
@@ -153,6 +154,9 @@
     # Only __init__ is called on startup. Asyncore is responsible for later
     # connections from the MTA.  slice and numslices are ignored and are
     # necessary only to satisfy the API.
+    
+    is_non_queue_runner = True
+    
     def __init__(self, slice=None, numslices=1):
         localaddr = config.mta.lmtp_host, int(config.mta.lmtp_port)
         # Do not call Runner's constructor because there's no QDIR to create
@@ -181,6 +185,7 @@
         # Do basic post-processing of the message, checking it for defects or
         # other missing information.
         message_id = msg.get('message-id')
+        message_store = getUtility(IMessageStore)
         if message_id is None:
             return ERR_550_MID
         if msg.defects:
 
=== modified file 'src/mailman/runners/rest.py'
--- src/mailman/runners/rest.py	2013-01-01 14:05:42 +0000
+++ src/mailman/runners/rest.py	2013-01-17 07:44:42 +0000
@@ -41,6 +41,7 @@
 
 
 class RESTRunner(Runner):
     intercept_signals = False
+    is_non_queue_runner = True
 
     def run(self):
         log.info('Starting REST server')
 
=== modified file 'src/mailman/runners/tests/test_lmtp.py'
--- src/mailman/runners/tests/test_lmtp.py	2013-01-01 14:05:42 +0000
+++ src/mailman/runners/tests/test_lmtp.py	2013-01-17 07:44:42 +0000
@@ -27,9 +27,11 @@
 
 import smtplib
 import unittest
+import os
 
 from datetime import datetime
 
+from mailman.config import config
 from mailman.app.lifecycle import create_list
 from mailman.database.transaction import transaction
 from mailman.testing.helpers import get_lmtp_client, get_queue_messages
@@ -109,3 +111,8 @@
         self.assertEqual(len(messages), 1)
         self.assertEqual(messages[0].msgdata['received_time'],
                          datetime(2005, 8, 1, 7, 49, 23))
+                         
+    def test_lmtp_queue_directory(self):
+        # Lmtp is a non queue runner, so it should not have a directory in var/queue
+        is_directory = os.path.isdir(os.path.join(config.paths['QUEUE_DIR'],'lmtp'))     
+        self.assertEqual(is_directory,False)