classification
Title: Add context management to mailbox.Mailbox
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: barry, ncoghlan, r.david.murray, sblondon, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-12-06 15:28 by sblondon, last changed 2017-12-10 23:11 by barry.

Pull Requests
URL Status Linked Edit
PR 4770 open sblondon, 2017-12-09 16:41
Messages (6)
msg307744 - (view) Author: (sblondon) * Date: 2017-12-06 15:28
mailbox.Mailbox has a .close() method that should be called at the end of Mailbox use.
I think it would be nice to use Mailbox instances with a 'with' statement so .close() will be called it automatically. So there is no need to check for which format it's required (yes for mbox, no for Maildir, etc.)

So the source code:
mbox = mailbox.mbox("/path/to/mbox")
...
mbox.close()

could become:
with mailbox.mbox("/path/to/mbox") as mbox:
    ...
msg307745 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-12-06 15:45
Yes, I think this is a good idea.  Would you like to submit a PR for it?

FWIW, we have this code in Mailman 3:

class Mailbox(MMDF):
    """A mailbox that interoperates with the 'with' statement."""

    def __enter__(self):
        self.lock()
        return self

    def __exit__(self, *exc):
        self.flush()
        self.unlock()
        # Don't suppress the exception.
        return False
msg307752 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-12-06 16:48
There is an ambiguity. What should the context manager do? Should it call a close() on exit (as the OP implies)? Or call lock() on enter and unlock() on exit as in Barry's example? Both behaviors look reasonable.

"In the face of ambiguity, refuse the temptation to guess" and "Explicit is better than implicit". This is perhaps the reason why this feature is not implemented yet. Perhaps there were discussions about this in the past.

contextlib.closing() can be used for explicit requesting the first behavior. Maybe it's a time to add contextlib.locked() or something like.
msg307759 - (view) Author: (sblondon) * Date: 2017-12-06 17:57
Currently, the implementation of .close() methods call flush() and
unlock() if it's needed.

About the ambiguity for the enter call, I think the context manager
should provide access to Mailbox, not a lock. If a lock is needed, we
could imagine another context manager:
with mailbox.Mailbox as mbox:
   #read messages, ...
   with mbox.lock():
      #actions that need lock

Do you think it's better?
msg307780 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-12-07 00:17
I don't know the mailbox API particularly well, but the fact the object offers all three of lock(), unlock() and close() as methods does imply a fair bit of inherent ambiguity.

One option would be to offer a module level "mailbox.locked()" API to handle the lock/unlock case, and then have native context management on the mailbox itself that was akin to "contextlib.closing".


(Note regarding contextlib.locked(): the reason we don't currently have that is because there's no consistent protocol for locking method names. acquire()/release() and lock()/unlock() would be the most popular pairings, so we could technically offer both "contextlib.acquired()" and "contextlib.locked()", but the duplication still seems a bit dubious to me)
msg308001 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-12-10 23:11
It's possible that the Mailman example can just assume that the mailbox will be flushed and unlocked on __exit__(), so it could just call .close().  Then the question is whether entering the CM should lock the mailbox.  The two cases are:

1. It doesn't, so you'd have to do:

with mbox(...) as mb:
    mb.lock()
    # ...

2. It does, so if for some reason you didn't want the lock, you'd have to:

with mbox(...) as mb:
    mb.unlock()

We *could* add a `lock` argument to the constructor to take the ambiguity away.  But I would claim that it doesn't make much sense to acquire an mbox in a CM and *not* lock it.  The idiom says to me "I want to do things to the mbox, exclusively, and if that requires locking it, JFDI".

So I would argue that the __enter__() should acquire the lock by default if the underlying mailbox supports it.
History
Date User Action Args
2017-12-10 23:11:59barrysetmessages: + msg308001
2017-12-09 17:18:57yselivanovsetnosy: - yselivanov
2017-12-09 16:41:40sblondonsetkeywords: + patch
stage: patch review
pull_requests: + pull_request4673
2017-12-07 00:17:36ncoghlansetmessages: + msg307780
2017-12-06 17:57:04sblondonsetmessages: + msg307759
2017-12-06 16:48:07serhiy.storchakasetnosy: + serhiy.storchaka, ncoghlan, yselivanov, r.david.murray
messages: + msg307752
2017-12-06 15:45:38barrysetnosy: + barry

messages: + msg307745
versions: + Python 3.7
2017-12-06 15:28:26sblondoncreate