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&vie...
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: foo@example.com\n' + 'X-Zope-To: bar@example.com, baz@example.com\n' + '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