[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