Working on this. tehre are some things I will get back to you about, because what you suggest breaks the unit test code...
Cheers, Matthew On Wed, 2008-03-19 at 23:32 +0200, Marius Gedminas wrote: > I'm starting a new thread, because the original one got hijacked by the > doctest/unittest flamefest. > > I appear to have unsubscribed from zope3-checkins a while ago, which is > why I'm posting here. > > Disclaimer: I'm not very good at doing code reviews. I tend to mention > every little detail that could've been improved and forget to praise > everything that's already perfect. > > There are two checkins on the branch. > > ========= > rev 84645 > ========= > http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84645&view=rev > > Matthew, Subversion thinks the two PDF files you added are plain-text > (svn:eol-style is "native", and svn diff shows the actual contents of > the PDFs, which isn't very useful to human readers). I would suggest > > svn propdel svn:eol-style *.pdf > svn propset svn:mime-type application/pdf *.pdf > > Also, the Quue_*.pdf seems to have a typo in the file name. > > At first sight those diagrams look scary and complicated. I'd look into > ways to express their content in a text file with plain-text > explanations and ASCII-art diagrams. > > ========= > rev 84551 > ========= > http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84551&view=rev > > > Index: src/zope/sendmail/configure.zcml > > =================================================================== > > --- src/zope/sendmail/configure.zcml (revision 84550) > > +++ src/zope/sendmail/configure.zcml (revision 84551) > > @@ -14,11 +14,18 @@ > > > > <!-- > > To send mail, uncomment the following directive and be sure to > > - create the queue directory. > > + create the queue directory. > > + > > + Other parameters sepcify the polling interval, and the retry > > "sepcify"? > > > + interval used when a temporary failure is detected. Lock links > > + can also be cleared on server start. > > > > <mail:queuedDelivery permission="zope.SendMail" > > queuePath="./queue" > > - mailer="smtp" /> > > + retryInterval="300" > > + pollingInterval="3000" > > + cleanLockLinks="False" > > + mailer="smtp" /> > > --> > > Please avoid literal tab characters in source files. > > > Index: src/zope/sendmail/zcml.py > > =================================================================== > > --- src/zope/sendmail/zcml.py (revision 84550) > > +++ src/zope/sendmail/zcml.py (revision 84551) > > @@ -70,7 +70,35 @@ class IQueuedDeliveryDirective(IDelivery > > description=u"Defines the path for the queue directory.", > > required=True) > > > > -def queuedDelivery(_context, permission, queuePath, mailer, name="Mail"): > > + pollingInterval = Int( > > + title=u'Polling Interval', > > + description=u'Defines the polling interval for queue processing in' > > + ' milliseconds', > > + default=3000, > > + required=False) > ... > > - thread = QueueProcessorThread() > > + thread = QueueProcessorThread(interval=pollingInterval/1000, > > You're using integer division here and losing sub-second precision. If you > don't want that, write interval=pollingInterval/1000.0. > > > + retry_interval=retryInterval, > > + clean_lock_links=cleanLockLinks) > > thread.setMailer(mailerObject) > > thread.setQueuePath(queuePath) > > thread.start() > > > Index: src/zope/sendmail/mailer.py > > =================================================================== > > --- src/zope/sendmail/mailer.py (revision 84550) > > +++ src/zope/sendmail/mailer.py (revision 84551) > ... > > @@ -41,22 +49,105 @@ class SMTPMailer(object): > > self.password = password > > self.force_tls = force_tls > > self.no_tls = no_tls > > + self.logger = None > > > > - def send(self, fromaddr, toaddrs, message): > > - connection = self.smtp(self.hostname, str(self.port)) > > + def set_logger(self, logger): > > + if not isinstance(logger, logging.Logger): > > + raise DoesNotImplement('Invalid logger given') > > I don't think DoesNotImplement is right here -- there are no interfaces > you're looking for. Better use the built-in TypeError. > > But is the type check necessary at all? The Pythonic thing is to rely on duck > typing. > > > + self.logger = logger > > + > > + def _log(self, log_level, *args): > > + if self.logger is None: > > + return > > + # This is not elegant, but it can be fixed later > > In my experience "later" usually doesn't happen. Why not fix it now? > > > + if log_level == 'debug': > > + self.logger.debug(*args) > > + elif log_level =='info': > > + self.logger.info(*args) > > + elif log_level =='error': > > + self.logger.error(*args) > > + elif log_level =='warning': > > + self.logger.warning(*args) > > + > > Just use > > self.logger.log(log_level, *args) > > and logging.DEBUG/logging.INFO/logging.ERROR/logging.WARNING as the > values for log_level. > > > + def _handle_smtp_error(self, > > + smtp_code, > > + smtp_error, > > + fromaddr, > > + toaddrs, > > + message_id): > > + """Process results of an SMTP error > > + returns True to indicate break needed""" > > The standard docstring convention is to do this: > > """Process results of an SMTP error. > > Returns True to indicate break needed. > """ > > One sentence on the first line, then an empty line, then additional > paragraphs after that. The closing """ on a separate line. All > sentences terminated by periods. > > > + def send(self, fromaddr, toaddrs, message, message_id): > > + try: > > + connection = self.smtp(self.hostname, str(self.port)) > > + except socket.error, e: > > + self._log('info', > > + "%s - SMTP server %s:%s - could not connect()," > > + " %s.", > > + message_id, > > + self.hostname, > > + str(self.port), > > + str(e),) > > + # temporary failure, go and sleep for > > + # retry_interval > > + raise MailerTemporaryFailureException() > > > > # send EHLO > > code, response = connection.ehlo() > > if code < 200 or code >= 300: > > code, response = connection.helo() > > if code < 200 or code >= 300: > > - raise RuntimeError('Error sending HELO to the SMTP server ' > > - '(code=%s, response=%s)' % (code, > > response)) > > + self._log('warning', > > + SMTP_ERR_MEDIUM_LOG_MSG, > > + message_id, > > + code, > > + str(response), > > + 'error sending HELO') > > + raise MailerTemporaryFailureException() > > > > # encryption support > > - have_tls = connection.has_extn('starttls') > > + have_tls = connection.has_extn('starttls') > > if not have_tls and self.force_tls: > > - raise RuntimeError('TLS is not available but TLS is required') > > + error_str = 'TLS is not available but TLS is required' > > + self._log('warning', > > + SMTP_ERR_SERVER_LOG_MSG, > > + message_id, > > + self.hostname, > > + self.port, > > + error_str) > > + raise MaileriTemporaryFailure(error_str) > > Typo: "Mailer*i*TemporaryFailure". Also an indication that this code > branch is not tested, otherwise the typo would've been caught. > > > + except smtplib.SMTPRecipientsRefused, e: > > + # This exception is raised because no recipients > > + # were acceptable - lets take the most common error > > + # code and proceed with that > > + freq = {} > > + for result in e.recipients.values(): > > + if freq.has_key(result): > > + freq[result] += 1 > > + else: > > + freq[result] = 1 > > + max_ = 0 > > + for result in freq.keys(): > > + if freq[result] > max_: > > + most_common = result > > + max_ = freq[result] > > + (smtp_code, smtp_error) = most_common > > Please extract this bit into a separate function. The send method is > already way too long and complicated. > > Once it's extracted an tested with an isolated test, you can refactor > it to something like > > freq = collections.defaultdict(int) > for smtp_code, smtp_error in e.recipients.values(): > freq[smtp_code, smtp_error] += 1 > smtp_code, smtp_error = max(freq, key=freq.get) > > Oh, wait, that only works on Python 2.5. Scratch the refactoring. > > > > + # Log ANY errors > > + if send_errors is not None: > > If an exception happens in this bit above: > > > + try: > > + send_errors = connection.sendmail(fromaddr, toaddrs, message) > > + except smtplib.SMTPSenderRefused, e: > > then send_errors will be undefined and you'll get a NameError. > Currently this is mostly not a problem, because almost all except > clauses for that try re-raise a different exception (either directly, or > via _handle_smtp_error). > > I suggest adding > > send_errors = None > > above the try:, just in case. > > > + # Log ANY errors > > + if send_errors is not None: > > + sentaddrs = [x for x in toaddrs > > + if x not in send_errors.keys()] > > + for address in send_errors.keys(): > > + self._log('warning', > > + SMTP_ERR_LOG_MSG, > > + message_id, > > + str(send_errors[address][0]), > > + send_errors[address][1], > > + fromaddr, > > + address) > > Suggest rewriting as > > sentaddrs = [x for x in toaddrs > if x not in send_errors] > for address, (smtp_code, smtp_error) in send_errors.items(): > self._log('warning', > SMTP_ERR_LOG_MSG, > message_id, > str(smtp_code), > smtp_error, > fromaddr, > address) > > (Also note that you don't have to explicitly call str() on all ints you > pass to a message; '%s' % 42 works perfectly fine.) > > > + else: > > + sentaddrs = list(toaddrs) > > > > Index: src/zope/sendmail/tests/test_mailer.py > > =================================================================== > > --- src/zope/sendmail/tests/test_mailer.py (revision 84550) > > +++ src/zope/sendmail/tests/test_mailer.py (revision 84551) > ... > > class SMTP(object): > > + _exception = None > > + _exception_args = None > > + _send_errors = None > > > > def __init__(myself, h, p): > > myself.hostname = h > > myself.port = p > > + self.smtp = myself > > if type(p) == type(u""): > > raise socket.error("Int or String expected") > > - self.smtp = myself > > + if p == '0': > > + raise socket.error("Emulated connect() failure") > > > > def sendmail(self, f, t, m): > > self.fromaddr = f > > self.toaddrs = t > > self.msgtext = m > > + if hasattr(self, '_exception'): > > + if self._exception and issubclass(self._exception, > > Exception): > > + if hasattr(self, '_exception_args') \ > > + and type(self._exception_args) is tuple: > > + raise > > self._exception(*self._exception_args) > > + else: > > + raise self._exception('Crazy Arguments > > WANTED!') > > That's a bit of an overkill. If you change the _exception_args default > value to () above, it would be perfectly fine to replace this lats bit > with > > if self._exception: > raise self._exception(*self._exception_args) > > It's the job of the Python interpreter to check the suitability of > _exception and _exception_args for the raise statement, you don't have > to do everything by hand. > > > + if self._send_errors: > > + return self._send_errors > ... > > + def test_set_logger(self): > > + # Do this with throw away instances... > > + test_mailer = SMTPMailer() > > + log_object = logging.getLogger('test_logger') > > + test_mailer.set_logger(log_object) > > + self.assertEquals(isinstance(test_mailer.logger, logging.Logger), > > True) > > Why not > > self.assertTrue(test_mailer.logger is log_object) > > instead? > > > def test_send(self): > > for run in (1,2): > > if run == 2: > > self.setUp(u'25') > > - fromaddr = '[EMAIL PROTECTED]' > > - toaddrs = ('[EMAIL PROTECTED]', '[EMAIL PROTECTED]') > > - msgtext = 'Headers: headers\n\nbodybodybody\n-- \nsig\n' > > - self.mailer.send(fromaddr, toaddrs, msgtext) > > - self.assertEquals(self.smtp.fromaddr, fromaddr) > > - self.assertEquals(self.smtp.toaddrs, toaddrs) > > - self.assertEquals(self.smtp.msgtext, msgtext) > > + else: > > + self.setUp() > > Pointless, the unittest framework calls self.setUp() for you > automatically. > > > + def test_mailer_no_connect(self): > > + # set up test value to raise socket.error exception > > + self.setUp('0') > > + try: > > + self.mailer.send(**self.test_kwargs) > > + except MailerTemporaryFailureException: > > + pass > > You want > > self.assertRaises(MailerTemporaryFailureException, > self.mailer.send, **self.test_kwargs) > > here (and elsewhere) > > > + self.assertEquals(self.mailer.logger.infos, > > > > Index: src/zope/sendmail/tests/test_delivery.py > > =================================================================== > > --- src/zope/sendmail/tests/test_delivery.py (revision 84550) > > +++ src/zope/sendmail/tests/test_delivery.py (revision 84551) > > @@ -21,6 +21,7 @@ $Id$ > ... > > +from socket import error as socket_error > > What's the advantage of this over > > import socket > > and then using socket.error elsewhere? > > > + def test_unlink(self): > > + self.thread.log = LoggerStub() # Clean log > > + self.filename = os.path.join(self.dir, 'message') > > + self.tmp_filename = os.path.join(self.dir, '.sending-message') > > + temp = open(self.filename, "w+b") > > + temp.write('X-Zope-From: [EMAIL PROTECTED]' > > + 'X-Zope-To: [EMAIL PROTECTED], [EMAIL PROTECTED]' > > + 'Header: value\n\nBody\n') > > + temp.close() > > + self.md.files.append(self.filename) > > + os.link(self.filename, self.tmp_filename) > > Is os.link available on Windows? > > > + self.thread._unlinkFile(self.tmp_filename) > > + self.failUnless(os.path.exists(self.filename)) > > + self.failIf(os.path.exists(self.tmp_filename), 'File exists') > > > Index: src/zope/sendmail/maildir.py > > =================================================================== > > --- src/zope/sendmail/maildir.py (revision 84550) > > +++ src/zope/sendmail/maildir.py (revision 84551) > ... > > + def _cleanLockLinks(self): > > + """Clean the maildir of any .sending-* lock files""" > > + # Fish inside the Maildir queue directory to get > > + # .sending-* files > > + # Get The links > > + join = os.path.join > > + subdir_cur = join(self.path, 'cur') > > + subdir_new = join(self.path, 'new') > > + lock_links = [join(subdir_new, x) for x in os.listdir(subdir_new) > > + if x.startswith(SENDING_MSG_LOCK_PREFIX)] > > + lock_links += [join(subdir_cur, x) for x in os.listdir(subdir_cur) > > + if x.startswith(SENDING_MSG_LOCK_PREFIX)] > > + # Remove any links > > + for link in lock_links: > > + try: > > + os.unlink(link) > > + except OSError, e: > > + if e.errno == 2: # file does not exist > > If you import errno, you can write > > if e.errno = errno.ENOENT: > > instead. I find that a bit clearer. > > > + # someone else unlinked the file; oh well > > + pass > > + else: > > + # something bad happend > > + raise > > > Index: src/zope/sendmail/interfaces.py > > =================================================================== > > --- src/zope/sendmail/interfaces.py (revision 84550) > > +++ src/zope/sendmail/interfaces.py (revision 84551) > ... > > +# > > +# Exception classes for use within Zope Sendmail, for use of Mailers > > +# > > +class IMailerFailureException(IException): > > + """Failure in sending mail""" > > + pass > > The 'pass' is not needed here. > > > +class MailerFailureException(Exception): > > + """Failure in sending mail""" > > + > > + implements(IMailerFailureException) > > + > > + def __init__(self, message="Failure in sending mail"): > > + self.message = message > > + self.args = (message,) > > Is the __init__ needed? Exception.__init__ will do essentially the same > thing. > > > + > > +class IMailerTemporaryFailureException(IMailerFailureException): > > + """Temporary failure in sending mail - retry later""" > > + pass > > + > > +class MailerTemporaryFailureException(MailerFailureException): > > + """Temporary failure in sending mail - retry later""" > > + > > + implements(IMailerTemporaryFailureException) > > + > > + def __init__(self, message="Temporary failure in sending mail - retry > > later"): > > + self.message = message > > + self.args = (message,) > > + > > + > > +class IMailerPermanentFailureException(IMailerFailureException): > > + """Permanent failure in sending mail - take reject action""" > > + pass > > + > > +class MailerPermanentFailureException(MailerFailureException): > > + """Permanent failure in sending mail - take reject action""" > > + > > + implements(IMailerPermanentFailureException) > > + > > + def __init__(self, message="Permanent failure in sending mail - take > > reject action"): > > + self.message = message > > + self.args = (message,) > > + > > > > class IMailer(Interface): > > - """Mailer handles synchronous mail delivery.""" > > + """Mailer handles synchronous mail delivery. > > > > - def send(fromaddr, toaddrs, message): > > + Mailer can raise the exceptions > > + > > + MailerPermanentFailure > > + MailerTemporaryFailure > > Well, no, your exception classes are named > MailerTemporaryFailureException and MailerPermanentFailureException. > > (I'd prefer the shorter names, personally. Maybe MailerTemporaryError > and MailerPermanentError, because FooError is the common naming > convention for exceptions.) > > > + > > + to indicate to sending process what action to take. > > + """ > > + > > + def send(fromaddr, toaddrs, message, message_id): > > """Send an email message. > > > > `fromaddr` is the sender address (unicode string), > > @@ -138,12 +203,18 @@ class IMailer(Interface): > > 2822. It should contain at least Date, From, To, and Message-Id > > headers. > > > > + `message_id` is an id for the message, typically a filename. > > + > > The naming clash with the Message-ID header (which has got *absolutely > nothing* to do with this message_id argument) is unfortunate. I think > this should be clarified in the docstring. No, I think someone should > go through the source code and replace message_id with queue_id > everywhere where it refers to the os.path.basename(file_in_queue) and > not to the Message-ID header. > > > Messages are sent immediatelly. > > > > Dispatches an `IMailSentEvent` on successful delivery, otherwise an > > `IMailErrorEvent`. > > """ > > > > + def set_logger(logger): > > + """Set the log object for the Mailer - this is for use by > > + QueueProcessorThread to hand a logging object to the mailer > > + """ > > Docstring formatting convention. > > """Set the logger for additional messages. > > The additional messages include information about SMTP > errors, connection timeouts, etc. > > The logger should be a logging.Logger object. If you pass > None, no messages will be logged. > """ > > Also, it's conventional to leave two blank lines between class and > interface declarations. (PEP-8 says this.) > > > class ISMTPMailer(IMailer): > > """A mailer that delivers mail to a relay host via SMTP.""" > > > > Index: src/zope/sendmail/delivery.py > > =================================================================== > > --- src/zope/sendmail/delivery.py (revision 84550) > > +++ src/zope/sendmail/delivery.py (revision 84551) > > @@ -223,6 +241,8 @@ class QueueProcessorThread(threading.Thr > > self.maildir = Maildir(path, True) > > > > def setMailer(self, mailer): > > + if not(IMailer.providedBy(mailer)): > > + raise (DoesNotImplement) > > This doesn't look like Python -- all those extra parens. > > if not IMailer.providedBy(mailer): > raise TypeError('not an IMailer', mailer) > > > self.mailer = mailer > > > > def _parseMessage(self, message): > ... > > + def _unlinkFile(self, filename): > > + """Unlink the message file """ > > + try: > > + os.unlink(filename) > > + except OSError, e: > > + if e.errno == 2: # file does not exist > > + # someone else unlinked the file; oh well > > + pass > > + else: > > + # something bad happend, log it > > + raise > > I've seen this function before. Can you merge all the copies into a > global function, say, called safe_delete()? > > > + def _queueRetryWait(self, tmp_filename, forever): > > + """Implements Retry Wait if there is an SMTP Connection > > + Failure or error 4xx due to machine load etc > > Blank line after the first one, then no extra indentation. > > > + """ > > + # Clean up by unlinking lock link > > + self._unlinkFile(tmp_filename) > > + # Wait specified retry interval in stages of self.interval > > + count = self.retry_interval > > + while(count > 0 and not self.__stopped): > > + if forever: > > + time.sleep(self.interval) > > + count -= self.interval > > Interesting. But lose the unnecessary parens after the while, at least. > > Is there any point to enter the loop if the forever arg is False? > > > + # Plug for test routines so that we know we got here > > + if not forever: > > + self.test_results['_queueRetryWait'] \ > > + = "Retry timeout: %s count: %s" \ > > + % (str(self.retry_interval), str(count)) > > Ouch. I think monkeypatching this method would be cleaner. > > > def run(self, forever=True): > > atexit.register(self.stop) > > + # Clean .sending- lock files from queue > > + if self.clean_lock_links: > > + self.maildir._cleanLockLinks() > > + # Set up logger for mailer > > + if hasattr(self.mailer, 'set_logger'): > > + self.mailer.set_logger(self.log) > > The interface requires all mailers to have a set_logger(), so why the > hasattr test? > > > @@ -263,8 +320,9 @@ class QueueProcessorThread(threading.Thr > > fromaddr = '' > > toaddrs = () > > head, tail = os.path.split(filename) > > - tmp_filename = os.path.join(head, '.sending-' + tail) > > - rejected_filename = os.path.join(head, '.rejected-' + tail) > > + tmp_filename = os.path.join(head, SENDING_MSG_LOCK_PREFIX > > + tail) > > + rejected_filename = os.path.join(head, REJECTED_MSG_PREFIX > > + tail) > > + message_id = os.path.basename(filename) > > try: > > # perform a series of operations in an attempt to > > ensure > > # that no two threads/processes send this message > > @@ -339,53 +397,46 @@ class QueueProcessorThread(threading.Thr > > file.close() > > fromaddr, toaddrs, message = > > self._parseMessage(message) > > try: > > - self.mailer.send(fromaddr, toaddrs, message) > > Oh, the message_id argument to mailer.send() is new! Please, please, > please, call it queue_id, and not message_id. > > > - except smtplib.SMTPResponseException, e: > > - if 500 <= e.smtp_code <= 599: > > - # permanent error, ditch the message > > - self.log.error( > > - "Discarding email from %s to %s due to" > > - " a permanent error: %s", > > - fromaddr, ", ".join(toaddrs), str(e)) > > - os.link(filename, rejected_filename) > > - else: > > - # Log an error and retry later > > - raise > > + sentaddrs = self.mailer.send(fromaddr, > > + toaddrs, > > + message, > > + message_id) > > + except MailerTemporaryFailureException, e: > > + self._queueRetryWait(tmp_filename, forever) > > + # We break as we don't want to send message later > > + break; > > No semicolons please. > > > + except MailerPermanentFailureException, e: > > + os.link(filename, rejected_filename) > > + sentaddrs = [] > > > Overall I like the new features *a lot*. The stylistic issues are > mostly minor. The most important of those are > > * Fix the integer division bug (easy, but a unit test would be good). > > * Resolve the exception name confusion (easy). > > * Clearly distinguish queue IDs from RFC-2822 message IDs (should be easy). > > And I have a questions: > > * What's up with retry_interval? _queueRetryWait doesn't do anything > in the inner loop, after sleeping for retry_interval seconds -- it > just checks self.__stopped. I suppose this makes Zope 3 shutdown > (or restart) faster, but the name seems to be wrong -- there are no > actual retries made after retry_interval seconds. > > Thank you very much for diving into the murky depths and improving > zope.sendmail! > > Marius Gedminas
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )