[Zope-dev] Re: zope.sendmail grantma-retryfixes branch review
Matthew Grant
grantma at anathoth.gen.nz
Mon Mar 24 18:42:59 EDT 2008
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: foo at example.com\n'
> > + 'X-Zope-To: bar at example.com, baz at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://mail.zope.org/pipermail/zope-dev/attachments/20080325/483988b1/attachment.bin
More information about the Zope-Dev
mailing list