On Thu, Jun 07, 2007 at 12:36:25AM +0300, Marius Gedminas wrote:
> On Wed, Jun 06, 2007 at 06:04:50PM +0200, Sascha Ottolski wrote:
> > I discovered a problem with the maildir implementation, that is 
> > triggered when trying to use the QueuedDelivery with many (read: more 
> > than current filedescriptor limit of the zope process) mails at once:
> ...
> > OSError: [Errno 24] Too many open files: 
> > '/home/so/sandbox/var/zope/lib/python/lalisio/app/site/../../../../var/zope/var/mqueue/new'
...
> > The reason for that to happen is clear to me, however, I'm not sure how
> > a fix could look like.
> 
> Did you create an bug in launchpad?  I could attach a patch that I think
> would fix the problem, but I don't currently have the time and energy to
> test myself.

I'm attaching the patch here.  I'd be happy to commit it to trunk if
somebody else could test it on various platforms.  (I haven't even
tested if it works fine on Linux, other than by running the unit tests.)

Marius Gedminas
-- 
(mental note: stop installing red hat. everytime i do so, it takes ages to fix
my system again.)
                -- from the sig of Martin H�gman
Make QueuedMailDelivery not keep open files around for every message sent
during a transaction.

Patch by Marius Gedminas <[EMAIL PROTECTED]>


Index: tests/test_maildir.py
===================================================================
--- tests/test_maildir.py	(revision 75881)
+++ tests/test_maildir.py	(working copy)
@@ -19,6 +19,7 @@
 import unittest
 import stat
 import os
+import errno
 
 from zope.interface.verify import verifyObject
 
@@ -123,7 +124,7 @@
     def open(self, filename, flags, mode=0777):
         if (flags & os.O_EXCL and flags & os.O_CREAT
             and self.access(filename, 0)):
-            raise OSError('file already exists')
+            raise OSError(errno.EEXIST, 'file already exists')
         if not flags & os.O_CREAT and not self.access(filename, 0):
             raise OSError('file not found')
         fd = max(self._descriptors.keys() + [2]) + 1
Index: tests/test_delivery.py
===================================================================
--- tests/test_delivery.py	(revision 75881)
+++ tests/test_delivery.py	(working copy)
@@ -147,18 +147,32 @@
     data = ''
     commited_messages = []  # this list is shared among all instances
     aborted_messages = []   # this one too
+    _closed = False
 
     def write(self, str):
+        if self._closed:
+            raise AssertionError('already closed')
         self.data += str
 
     def writelines(self, seq):
+        if self._closed:
+            raise AssertionError('already closed')
         self.data += ''.join(seq)
 
+    def close(self):
+        self._closed = True
+
     def commit(self):
+        if not self._closed:
+            raise AssertionError('for this test we want the message explicitly'
+                                 ' closed before it is committed')
         self._commited = True
         self.commited_messages.append(self.data)
 
     def abort(self):
+        if not self._closed:
+            raise AssertionError('for this test we want the message explicitly'
+                                 ' closed before it is committed')
         self._aborted = True
         self.aborted_messages.append(self.data)
 
Index: maildir.py
===================================================================
--- maildir.py	(revision 75881)
+++ maildir.py	(working copy)
@@ -18,6 +18,7 @@
 __docformat__ = 'restructuredtext'
 
 import os
+import errno
 import socket
 import time
 import random
@@ -91,7 +92,9 @@
             filename = join(subdir_tmp, unique)
             try:
                 fd = os.open(filename, os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600)
-            except OSError:
+            except OSError, e:
+                if e.errno != errno.EEXIST:
+                    raise
                 # File exists
                 counter += 1
                 if counter >= 1000:
@@ -115,7 +118,7 @@
         self._filename = filename
         self._new_filename = new_filename
         self._fd = fd
-        self._closed = False
+        self._finished = False
         self._aborted = False
 
     def write(self, data):
@@ -124,22 +127,28 @@
     def writelines(self, lines):
         self._fd.writelines(lines)
 
+    def close(self):
+        self._fd.close()
+
     def commit(self):
-        if self._closed and self._aborted:
+        if self._aborted:
             raise RuntimeError('Cannot commit, message already aborted')
-        elif not self._closed:
-            self._closed = True
-            self._aborted = False
+        elif not self._finished:
+            self._finished = True
             self._fd.close()
             os.rename(self._filename, self._new_filename)
             # NOTE: the same maildir.html says it should be a link, followed by
             #       unlink.  But Win32 does not necessarily have hardlinks!
 
     def abort(self):
-        if not self._closed:
-            self._closed = True
+        # XXX mgedmin: I think it is dangerous to have an abort() that does
+        # nothing when commit() already succeeded.  But the tests currently
+        # test that expectation.
+        if not self._finished:
+            self._finished = True
             self._aborted = True
             self._fd.close()
             os.unlink(self._filename)
 
-    # should there be a __del__ that does abort()?
+    # XXX: should there be a __del__ that calls abort()?
+
Index: interfaces.py
===================================================================
--- interfaces.py	(revision 75881)
+++ interfaces.py	(working copy)
@@ -243,6 +243,15 @@
         any line separators.
         """
 
+    def close():
+        """Closes the message file.
+
+        No further writes are allowed.  You can call ``close()`` before calling
+        ``commit()`` or ``abort()`` to avoid having too many open files.
+
+        Calling ``close()`` more than once is allowed.
+        """
+
     def commit():
         """Commits the new message using the `Maildir` protocol.
 
Index: delivery.py
===================================================================
--- delivery.py	(revision 75881)
+++ delivery.py	(working copy)
@@ -134,6 +134,7 @@
         msg.write('X-Zope-From: %s\n' % fromaddr)
         msg.write('X-Zope-To: %s\n' % ", ".join(toaddrs))
         msg.write(message)
+        msg.close()
         return MailDataManager(msg.commit, onAbort=msg.abort)
 
 

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Zope3-dev mailing list
Zope3-dev@zope.org
Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com

Reply via email to