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, cheryl.sabella, ncoghlan, r.david.murray, sblondon, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-12-06 15:28 by sblondon, last changed 2018-03-28 21:06 by sblondon.

Pull Requests
URL Status Linked Edit
PR 4770 open sblondon, 2017-12-09 16:41
Messages (11)
msg307744 - (view) Author: Stéphane Blondon (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: Stéphane Blondon (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.
msg308559 - (view) Author: Stéphane Blondon (sblondon) * Date: 2017-12-18 14:04
If the access is read-only, the lock is not required [1].  However, I agree
it does not worth to be more complex (changing the signature of the
subclass or adding another context manager for the lock).
I updated the patch and documentation so the mailbox is locked at the
__enter__().

1:  For example, the first source code example at
https://docs.python.org/3/library/mailbox.html#examples
msg314253 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python triager) Date: 2018-03-22 13:24
It looks like Barry was ready to merge this PR in December.  Is there anything else that needs to be done for it?  Thanks!
msg314318 - (view) Author: Stéphane Blondon (sblondon) * Date: 2018-03-23 18:18
I don't know about something blocking the merge. I sent a message to Barry
on Github the 3th january but perhaps it was a wrong timing (too soon after
new year).
msg314319 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-23 18:52
Is closing a mailbox in __exit__ the most desirable operation?

In the last example on https://docs.python.org/3/library/mailbox.html#examples inbox is locked and unlocked multiple times. The with statement couldn't be used here.

On https://pymotw.com/3/mailbox/ some examples use the idiom

    mbox = ...
    mbox.lock()
    try:
        ...
    finally:
        mbox.unlock()

and others use the idiom

    mbox = ...
    mbox.lock()
    try:
        ...
    finally:
        mbox.flush()
        mbox.close()
msg314621 - (view) Author: Stéphane Blondon (sblondon) * Date: 2018-03-28 21:06
The availability of context manager does not make it mandatory if it does
not fit some needs.

> In the last example on https://docs.python.org/3/
> library/mailbox.html#examples inbox is locked and unlocked multiple
> times. The with statement couldn't be used here.
>

I agree with the idea: if the user code needs to manage a lock, using this
context manager is a bad idea.

By the way, this example does not need to manage the lock because 'inbox'
is an instance of mailbox.Maildir so the .lock() and .unlock() calls do
nothing for this class (
https://docs.python.org/3/library/mailbox.html#mailbox.Maildir.unlock).

>
> On https://pymotw.com/3/mailbox/ some examples use the idiom
>
>     mbox = ...
>     mbox.lock()
>     try:
>         ...
>     finally:
>         mbox.unlock()
>
> and others use the idiom
>
>     mbox = ...
>     mbox.lock()
>     try:
>         ...
>     finally:
>         mbox.flush()
>         mbox.close()
>

In the first example, there is a .flush() call at the end of the try block.
In the second case, mbox.flush() is unnecessary because .close() call
flush. So I see it like choosing between (.flush() and .unlock()) or
.close(). It's what the context manager does.

If there is no agreement, perhaps this proposal should be abandoned?
History
Date User Action Args
2018-03-28 21:06:29sblondonsetmessages: + msg314621
2018-03-23 18:52:41serhiy.storchakasetmessages: + msg314319
2018-03-23 18:18:48sblondonsetmessages: + msg314318
2018-03-22 13:24:01cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg314253
2017-12-18 14:04:06sblondonsetmessages: + msg308559
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