Skip to content

Comments

gh-116738: Add critical section to dbm/gdbm context manager#140391

Merged
colesbury merged 1 commit intopython:mainfrom
yoney:ft_dbm
Oct 22, 2025
Merged

gh-116738: Add critical section to dbm/gdbm context manager#140391
colesbury merged 1 commit intopython:mainfrom
yoney:ft_dbm

Conversation

@yoney
Copy link
Contributor

@yoney yoney commented Oct 20, 2025 •
edited
Loading

Add Py_{BEGIN,END}_CRITICAL_SECTION() to the context managers in the dbm and gdbm modules for the free threading build. Other methods are already covered.

This could be a problem in a rare corner case if a thread outlives the context manager's scope. I created a local test, and TSAN identifies the issue.

    def test_context_manager(self):
        def worker(db):
            db.close()

        with os_helper.temp_dir() as tmpdirname:
            with gdbm.open(f"{tmpdirname}/gdbm_filename", "c") as db:
                t1 = threading.Thread(target=worker, args=(db,))
                t1.start()

        t1.join()
WARNING: ThreadSanitizer: data race (pid=1672837)
  Read of size 8 at 0x7f9fdb330e48 by thread T1:
    #0 _gdbm_gdbm_close_impl /home/alper/workspace/tsan/./Modules/_gdbmmodule.c:413:15 (_gdbm.cpython-315td-x86_64-linux-gnu.so+0x2ce1) (BuildId: 42fff9900c3973fef01c9a778b42049ab0110b9f)
    #1 _gdbm_gdbm_close /home/alper/workspace/tsan/./Modules/clinic/_gdbmmodule.c.h:100:20 (_gdbm.cpython-315td-x86_64-linux-gnu.so+0x2ce1)
    #2 method_vectorcall_NOARGS /home/alper/workspace/tsan/Objects/descrobject.c:448:24 (python+0x5c662b) (BuildId: 89128bd333556519224b9af239856b3fcb80f8ab)
...

  Previous write of size 8 at 0x7f9fdb330e48 by main thread:
    #0 _gdbm_gdbm_close_impl /home/alper/workspace/tsan/./Modules/_gdbmmodule.c:416:18 (_gdbm.cpython-315td-x86_64-linux-gnu.so+0x4944) (BuildId: 42fff9900c3973fef01c9a778b42049ab0110b9f)
    #1 gdbm__exit__ /home/alper/workspace/tsan/./Modules/_gdbmmodule.c:693:12 (_gdbm.cpython-315td-x86_64-linux-gnu.so+0x4944)
    #2 method_vectorcall_VARARGS /home/alper/workspace/tsan/Objects/descrobject.c:325:24 (python+0x5c5ef9) (BuildId: 89128bd333556519224b9af239856b3fcb80f8ab)
...

SUMMARY: ThreadSanitizer: data race /home/alper/workspace/tsan/./Modules/_gdbmmodule.c:413:15 in _gdbm_gdbm_close_impl
==================

cc: @mpage @colesbury

@bedevere-app bedevere-app bot mentioned this pull request Oct 20, 2025
Open
@colesbury colesbury requested review from colesbury and mpage October 20, 2025 21:59
@colesbury colesbury added the needs backport to 3.14 bugs and security fixes label Oct 20, 2025
@yoney yoney marked this pull request as ready for review October 20, 2025 22:00
serhiy-storchaka
serhiy-storchaka approved these changes Oct 22, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

colesbury
colesbury approved these changes Oct 22, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well.

FYI, you can also use Argument Clinic for __enter__ and __exit__ functions now if you prefer Argument Clinic boilerplate to Py_BEGIN_CRITICAL_SECTION boilerplate.

@colesbury colesbury merged commit d51be28 into python:main Oct 22, 2025
53 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Oct 22, 2025

Thanks @yoney for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 22, 2025
@yoney @miss-islington
pythongh-116738: Add critical section to dbm/gdbm context manager (py…
556c3d5 
…thongh-140391)

(cherry picked from commit d51be28)

Co-authored-by: Alper <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 22, 2025

GH-140459 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Oct 22, 2025
colesbury pushed a commit that referenced this pull request Oct 22, 2025
@miss-islington @yoney
[3.14] gh-116738: Add critical section to dbm/gdbm context manager (g…
01b52ea 
…h-140391) (gh-140459)

(cherry picked from commit d51be28)

Co-authored-by: Alper <[email protected]>
@yoney yoney deleted the ft_dbm branch October 28, 2025 15:51
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@yoney @StanFromIreland
pythongh-116738: Add critical section to dbm/gdbm context manager (py…
055bca5 
…thongh-140391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@mpage mpage Awaiting requested review from mpage

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yoney @colesbury @serhiy-storchaka @kumaraditya303