FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection by marko1616 · Pull Request #123400 · python/cpython · GitHub

/ cpython Public

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

Closed
marko1616 wants to merge 3 commits into
python:mainfrom
marko1616:fix/memoryview-picklebuffer-circular-ref
Closed

gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection#123400
marko1616 wants to merge 3 commits into
python:mainfrom
marko1616:fix/memoryview-picklebuffer-circular-ref

Conversation

marko1616 commented Aug 27, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown

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 commented Aug 27, 2024
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

bedevere-app Bot commented Aug 27, 2024

Copy link
Copy Markdown

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 Bot commented Aug 27, 2024

Copy link
Copy Markdown

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.

Comment thread Objects/memoryobject.c Outdated

bedevere-app Bot commented Aug 28, 2024

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
Member

cc @serhiy-storchaka

marko1616 force-pushed the fix/memoryview-picklebuffer-circular-ref branch from 8b19468 to f63d798 Compare August 29, 2024 14:52

bedevere-app Bot commented Aug 29, 2024

Copy link
Copy Markdown

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.

1 similar comment

bedevere-app Bot commented Sep 1, 2024

Copy link
Copy Markdown

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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
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 commented Sep 9, 2024
edited
Loading

Copy link
Copy Markdown
Author

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 force-pushed the fix/memoryview-picklebuffer-circular-ref branch from f1c9f77 to 7fe3839 Compare September 9, 2024 23:30

bedevere-app Bot commented Sep 9, 2024

Copy link
Copy Markdown

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 closed this Sep 10, 2024

Copy link
Copy Markdown
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 deleted the fix/memoryview-picklebuffer-circular-ref branch September 10, 2024 11:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL