bpo-32234: Context manager available for mailbox instances #4770

sblondon · 2017-12-09T16:41:39Z

This patch implements the with statement for all mailbox formats.
The test is limited to syntax and instance checking because there is no way to check if close() has been called. If you prefer, I can write a specific subclass of Mailbox or make a mock to check if close() has been called.

However, as I'm not sure there is a consensus about what it the requested behaviour at __exit__(), I consider it more as a first step.

I tried to find previous discussions in the bug tracker, without success. Bug 7944 is about some close() not called and suggests the use of with for the tests, not the interpreter. Another one (23865) is about idempotency for close(). Both are fixed.

https://bugs.python.org/issue32234

csabella · 2017-12-10T13:41:51Z

Should this be added to the documentation page (Doc/library/mailbox.rst)?

sblondon · 2017-12-10T21:19:51Z

Thank you for you remark @csabella.
I committed few lines in mailbox.rst about it.

warsaw

I claim that locking the underlying mailbox is the only reasonable semantic for entering the CM.

Also, please use blurb to add a NEWS entry. Other than that, I think this is a good addition. Thanks!

bedevere-bot · 2017-12-10T23:13:28Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

warsaw · 2017-12-10T23:14:40Z

Oh, and as for your test question: I don't think you need to mock close(). If you agree with the lock suggestion, then you can write the test to check that the mailbox is locked inside the CM and unlocked outside of it.

sblondon · 2017-12-15T08:19:08Z

I have made the requested changes; please review again

I don't understand how to check easily because there are no boolean (or equivalent) to know if the lock has been acquired.
I see several solutions:

implement a subclass of Maildir in the tests and check the context manager with this one.
use mock
the code is obvious, so there is no need to test it.

I didn't add the unlock() call in __exit__() because it's not necessary. The documentation says that close() 'unlock it if necessary'. The current implementations respect this behaviour. I agree to add it if you think it's better (because there will be a symetry: lock at enter, unlock at exit).

bedevere-bot · 2017-12-15T08:19:10Z

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

warsaw

This looks great. I have just a minor comment about the documentation. Please fix that and then I think we're good to go. Thanks for your contribution!

Doc/library/mailbox.rst

bedevere-bot · 2017-12-18T18:21:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

sblondon · 2017-12-18T20:14:22Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-18T20:14:24Z

Thanks for making the requested changes!

@warsaw: please review the changes made to this pull request.

sblondon · 2018-01-03T20:56:19Z

@warsaw Sorry to recall you about this issue. Should I do something more? Or you didn't have the time to review it and I just have to wait?

csabella · 2019-07-03T23:43:09Z

@maxking, please take a look at this one and the comments on the bug tracker to see if this can move forward. Thanks!

maxking

Overall I think this should be merged and is complete. I didn't see any objects/conflicts on BPO. If you can address the comments from @csabella and one small suggestion from me, we can get this merged.

Thanks for your contribution @sblondon !

Misc/NEWS.d/next/Library/2017-12-15-09-32-57.bpo-32234.XaOkhR.rst

matrixise

Thank you for your contribution, but could you add a test for the lock with the with statement?

matrixise · 2019-08-01T06:42:25Z

Lib/test/test_mailbox.py

+        # Mailboxes are usable as a context manager
+        with self._factory(self._path) as box:
+            self.assertIsInstance(box, self._box.__class__)
+


Could you add a test for the lock?

@matrixise I'm unsure how to do it right. For example:

Maildir doesn't use the lock. So the test is not useful there.

Mailbox uses it and stores the state in an internal boolean variable (self._lock). So the test would check the implementation.

It would be nice to have a locked attribute for Mailbox but it will not work for each case and is probably too invasive for this patch.

good question, I don't remember your code, but I am going to assign myself your PR.

Ok, thank you!

This can be tested in _TestSingleFile, which tests the classes that have _locked and _file attributes.

It looks like this test is redundant now with the new test.

The other test is only for the subclasses that have _locked & _file. This one tests that all Mailboxes are usable as a context manager.

Ah, that makes sense.

bedevere-bot · 2019-08-01T06:43:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…ondon/cpython into mailbox_with_context_manager

…lbox_with_context_manager

github-actions · 2022-08-13T00:13:26Z

This PR is stale because it has been open for 30 days with no activity.

encukou · 2026-02-10T14:00:39Z

This PR is stalled but looks ready to go, let's push it over the finish line now?

I updated, and took the liberty of touching up the tests, as @matrixse assigned himself for that.

bitdancer · 2026-02-10T20:21:30Z

Doc/library/mailbox.rst

+   The :class:`Mailbox` class supports the :keyword:`with` statement.  When used
+   like this, the Mailbox acquires a lock when the :keyword:`with` statement
+   enters and releases it and :meth:`close` when the :keyword:`with` statement
+   exits.


Suggested change

exits.

The :class:`Mailbox` class supports the :keyword:`with` statement. When used

as a context manager, :class:`!Mailbox` calls :meth:`lock` when the context is entered,

returns the mailbox object as the context object, and at context end calls :meth:`close`,

thereby releasing the lock.

bitdancer · 2026-02-10T20:21:35Z

Lib/test/test_mailbox.py

+        # Mailboxes are usable as a context manager
+        with self._factory(self._path) as box:
+            self.assertIsInstance(box, self._box.__class__)
+


It looks like this test is redundant now with the new test.

Misc/NEWS.d/next/Library/2017-12-15-09-32-57.bpo-32234.XaOkhR.rst

Doc/library/mailbox.rst

Co-authored-by: R. David Murray <[email protected]>

sblondon · 2026-02-12T09:40:58Z

Thanks everyone to restart working on this PR. I'm the first author of this PR. Do you expect something from me ?
I can rebase the patch on the last commit on python:main if you want.

encukou · 2026-02-12T10:09:48Z

Congratulations, you have one of the oldest still-open PRs in CPython :)

Do you expect something from me?

No, but thanks for the offer!
I've merged the main branch already (to get CI to run). The current problem is that docs linters are more nitpicky than in 2017, but to appease them I think it's best for me to push fixes to your branch.

encukou · 2026-02-12T10:14:31Z

@warsaw @matrixise, may I dismiss your "red" reviews?

Context manager available for mailbox instances

8d1a9aa

sblondon requested a review from a team as a code owner December 9, 2017 16:41

the-knights-who-say-ni added the CLA signed label Dec 9, 2017

bedevere-bot added the awaiting review label Dec 9, 2017

Add documentation about availability of 'with' statement

47fd0fe

warsaw requested changes Dec 10, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 10, 2017

Lock Mailbox when used in a context manager

ae45e88

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 15, 2017

add news about the context manager for Mailbox

382e57f

warsaw requested changes Dec 18, 2017

View reviewed changes

Doc/library/mailbox.rst Outdated Show resolved Hide resolved

bedevere-bot removed the awaiting change review label Dec 18, 2017

bedevere-bot added the awaiting changes label Dec 18, 2017

sblondon added 2 commits December 18, 2017 21:05

fix speling errors

fd4d240

fix spelling errors

3c68123

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 18, 2017

csabella requested a review from warsaw March 18, 2019 10:38

maxking suggested changes Jul 4, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2017-12-15-09-32-57.bpo-32234.XaOkhR.rst Outdated Show resolved Hide resolved

update versionchanged to 3.9; improve News sentence

755dc1c

matrixise requested changes Aug 1, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 1, 2019

Merge branch 'mailbox_with_context_manager' of https://github.com/sbl…

bb790e5

…ondon/cpython into mailbox_with_context_manager

matrixise self-assigned this Sep 13, 2019

Merge branch 'master' of https://github.com/sblondon/cpython into mai…

ac9f696

…lbox_with_context_manager

sblondon mannequin mentioned this pull request May 21, 2022

Add context management to mailbox.Mailbox #76415

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 13, 2022

encukou added 3 commits February 10, 2026 14:51

Merge in the main branch

2131714

For classes with _locked and _file, test locking & closing

6814da0

Simpler assertion that the manager produces itself

bef0829

github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 10, 2026

bitdancer requested changes Feb 10, 2026

View reviewed changes

encukou reviewed Feb 11, 2026

View reviewed changes

Doc/library/mailbox.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

49403ae

Co-authored-by: R. David Murray <[email protected]>

sblondon and others added 2 commits February 12, 2026 10:41

Merge branch 'python:main' into mailbox_with_context_manager

3582862

Fix docs nits

bbddd8a

bitdancer approved these changes Feb 12, 2026

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Feb 12, 2026

-   exits.
+   The :class:`Mailbox` class supports the :keyword:`with` statement.  When used
+   as a context manager, :class:`!Mailbox` calls :meth:`lock` when the context is entered,
+   returns the mailbox object as the context object, and at context end calls :meth:`close`,
+   thereby releasing the lock.

Uh oh!

bpo-32234: Context manager available for mailbox instances #4770

Are you sure you want to change the base?

bpo-32234: Context manager available for mailbox instances #4770

Conversation

sblondon commented Dec 9, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csabella commented Dec 10, 2017

Uh oh!

sblondon commented Dec 10, 2017

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 10, 2017

Uh oh!

warsaw commented Dec 10, 2017

Uh oh!

sblondon commented Dec 15, 2017

Uh oh!

bedevere-bot commented Dec 15, 2017

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Dec 18, 2017

Uh oh!

sblondon commented Dec 18, 2017

Uh oh!

bedevere-bot commented Dec 18, 2017

Uh oh!

sblondon commented Jan 3, 2018

Uh oh!

csabella commented Jul 3, 2019

Uh oh!

maxking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matrixise left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sblondon Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 1, 2019

Uh oh!

github-actions bot commented Aug 13, 2022

Uh oh!

encukou commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sblondon commented Feb 12, 2026

Uh oh!

encukou commented Feb 12, 2026

Uh oh!

encukou commented Feb 12, 2026

Uh oh!

sblondon commented Dec 9, 2017 •

edited by bedevere-bot

Loading

sblondon Aug 5, 2019 •

edited

Loading