Add context management to mailbox.Mailbox

[b786cac0-71df-4c35-a58b-012a049d8b3d]

BPO	32234
Nosy	@warsaw, @ncoghlan, @bitdancer, @serhiy-storchaka, @csabella, @sblondon
PRs	python/cpython#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: ```python assignee = None closed_at = None created_at = labels = ['3.7', 'type-feature', 'library'] title = 'Add context management to mailbox.Mailbox' updated_at = user = 'https://github.com/sblondon' ``` bugs.python.org fields: ```python activity = actor = 'sblondon' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = 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'] ```

[b786cac0-71df-4c35-a58b-012a049d8b3d]

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.

[b786cac0-71df-4c35-a58b-012a049d8b3d]

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.

[b786cac0-71df-4c35-a58b-012a049d8b3d]

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!

[b786cac0-71df-4c35-a58b-012a049d8b3d]

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

[b786cac0-71df-4c35-a58b-012a049d8b3d]

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?