Add context management to mailbox.Mailbox

[sblondon]

BPO	32234
Nosy	@warsaw, @ncoghlan, @bitdancer, @serhiy-storchaka, @csabella, @sblondon
PRs	bpo-32234: Context manager available for mailbox instances #4770

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-12-06.15:28:26.422>
labels = ['3.7', 'type-feature', 'library']
title = 'Add context management to mailbox.Mailbox'
updated_at = <Date 2018-03-28.21:06:29.958>
user = 'https://github.com/sblondon'

bugs.python.org fields:

activity = <Date 2018-03-28.21:06:29.958>
actor = 'sblondon'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2017-12-06.15:28:26.422>
creator = 'sblondon'
dependencies = []
files = []
hgrepos = []
issue_num = 32234
keywords = ['patch']
message_count = 11.0
messages = ['307744', '307745', '307752', '307759', '307780', '308001', '308559', '314253', '314318', '314319', '314621']
nosy_count = 6.0
nosy_names = ['barry', 'ncoghlan', 'r.david.murray', 'serhiy.storchaka', 'cheryl.sabella', 'sblondon']
pr_nums = ['4770']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32234'
versions = ['Python 3.7']

[sblondon]

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:
...

[warsaw]

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

[serhiy-storchaka]

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.

[sblondon]

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?

[ncoghlan]

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)

[warsaw]

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.

[sblondon]

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

[csabella]

It looks like Barry was ready to merge this PR in December. Is there anything else that needs to be done for it? Thanks!

[sblondon]

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).

[serhiy-storchaka]

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()

[sblondon]

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?