8000 gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection by marko1616 · Pull Request #123400 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection #123400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection #123400

wants to merge 3 commits into from

Conversation

marko1616
Copy link
@marko1616 marko1616 commented Aug 27, 2024 •
edited by bedevere-app bot
Loading

Issues

Main Causes

  1. Creating a circular reference through a constructed exception, preventing garbage collection of PickleBuffer and memory objects
  2. Calling gc.collect()
  3. During the collection of the memory object, PyMemoryViewObject->mbuf is cleared for the first time (mbuf is set to a null pointer)
  4. When collecting PickleBuffer, memory_dealloc is called again through picklebuf_clear, at which point an exception occurs because mbuf was set to a null pointer in step 3

Next

I have no idea if this is a good way to fix this.But thank for any dev who reviewed this PR.

@ghost
Copy link
ghost commented Aug 27, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot mentioned this pull request Aug 27, 2024
Closed
@bedevere-app
Copy link
bedevere-app bot commented Aug 27, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link
bedevere-app bot commented Aug 27, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

vstinner
View reviewed changes
@bedevere-app
Copy link
bedevere-app bot commented Aug 28, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@vstinner
Copy link
Member

cc @serhiy-storchaka

@marko1616 marko1616 force-pushed the fix/memoryview-picklebuffer-circular-ref branch from 8b19468 to f63d798 Compare August 29, 2024 14:52
@bedevere-app
Copy link
bedevere-app bot commented Aug 29, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

< 8000 path d="M15 8a7.002 7.002 0 00-7-7" stroke="currentColor" stroke-width="2" stroke-linecap="round" vector-effect="non-scaling-stroke" />

1 similar comment
@bedevere-app
Copy link
bedevere-app bot commented Sep 1, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

encukou
encukou approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member
@encukou encukou left a comment

Choose a reason for hiding this comment

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

I also don't know a good way to fix this, but a leak with an unraisable error message is better than a crash.

I plan to merge in about a week if there are no objections.

@serhiy-storchaka
Copy link
Member

This change needs a test. test_non_continuous_buffer in Lib/test/pickletester.py can be an indirect test (it should be modified for this), but since this change does not use pickle.PickleBuffer, it needs more direct test that only uses memoryview.

I am not sure that this is a right solution, but the test will help us to decide.

@marko1616
Copy link
Author
marko1616 commented Sep 9, 2024 •
edited
Loading

This change needs a test. test_non_continuous_buffer in Lib/test/pickletester.py can be an indirect test (it should be modified for this), but since this change does not use pickle.PickleBuffer, it needs more direct test that only uses memoryview.

I am not sure that this is a right solution, but the test will help us to decide.

It's really hard to trigger the crash using just memoryview because we first need to construct an exported buffer that will clear the view. I tried, but I failed.
I'm consider adding following test in Lib/test/pickletester.py.

    def test_circular_reference_gc(self):
        mv = memoryview(b"foobar")
        error = None
        unraisable_exception = None
    
        def custom_unraisablehook(unraisable):
            nonlocal unraisable_exception
            unraisable_exception = unraisable

        def func():
            try:
                pb = pickle.PickleBuffer(mv)
                raise CustomError
            except CustomError as e:
                # Ensure `error` references the exception
                error = e
        func()
        del mv
        prev_unraisablehook = sys.unraisablehook
        sys.unraisablehook = custom_unraisablehook
        support.gc_collect()
        self.assertEqual(unraisable_exception.exc_type, BufferError)
        self.assertIsInstance(unraisable_exception.exc_value, BufferError)
        self.assertEqual(str(unraisable_exception.exc_value), 'memoryview has 1 exported buffer')
        sys.unraisablehook = prev_unraisablehook

marko1616 and others added 3 commits September 10, 2024 07:30
@marko1616 marko1616 force-pushed the fix/memoryview-picklebuffer-circular-ref branch from f1c9f77 to 7fe3839 Compare September 9, 2024 23:30
@bedevere-app
Copy link
bedevere-app bot commented Sep 9, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@marko1616 marko1616 closed this Sep 10, 2024
@serhiy-storchaka
Copy link
Member

#123898 uses a different solution. Trying to break a reference loop is a normal situation, it should not produce error reports (the user cannot do anything about this in any case), so _memory_release() should not even be called if self->exports != 0. That PR also contains tests.

@marko1616 marko1616 deleted the fix/memoryview-picklebuffer-circular-ref branch September 10, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@encukou encukou encukou approved these changes

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marko1616 @vstinner @serhiy-storchaka @encukou
0