Hi! Just done edits you suggested. Some Unit tests still to be written. Won't be doing one for that /1000 as tehre are more important things to check like exception functionality.
On Wed, 2008-03-19 at 23:32 +0200, Marius Gedminas wrote: > 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. I am paid to write code, not spend hours messing with documentation tools. Scans of my pencil diagrams are all that I can do because of time constraints. Better than nothing. > 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) > > > > 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. Left this as I can't get what you suggest to work - this is only test code. I think this is there because this stub is used in many places,and this saves gobs of repeated test code. Due to time I can't look any further into this. > > + 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? Don't think it is natively.... but they may have an abortion/aberrative thing to POSIXise the API! > > Index: src/zope/sendmail/delivery.py > > =================================================================== > > --- src/zope/sendmail/delivery.py (revision 84550) > > +++ src/zope/sendmail/delivery.py (revision 84551) > ... > > + 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()? > You probably have seen this in another part of delivery.py that is very similar? Around os.link etc.... I have taken out all the exatly common code to that above if read delivery.py closely. > > + 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. We want to test that this delay code is functional or we WILL spam the log, and try to send messages every 3 seconds! This ugliness is neede to get unit testing to this depth. > > 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? > Sorry, I have left this! > > 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). > All done > 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. If read code closely and look at state machine diagrams you will notice that .sending- links are removed before entering retry wait, ready for the sending process to restart and retry sending unsent queued messages! > Thank you very much for diving into the murky depths and improving > zope.sendmail! It 's been a pleasure. This review is also improving our code base quality, while getting these much needed changes out there! There is still a fault with zope.sendmail when it comes to sending a single email message to multiple recipients. To do it properly, the send state of the message for each recipient should be tracked. This would necessitate a major rewrite. The code as it is should handle sending such messages to a Smarthost if the Smarthost MTA is configured correctly to receive messages without content checking from Zope 3. I suggest that another mailer using the /usr/sbin/sendmail or /usr/lib/sendmail binaries be used to get around this. I know that Windows does not have this, and that some system administrators will be cheapskates and try direct sending, but that's life! Cheers, Matthew Grant
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 )