Skip to content

bpo-42972: Fix sqlite3 traverse/clear #26452

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.

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

Merged
merged 1 commit into from
May 31, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 29, 2021

GH-26104 had some issues:

  • members were not visited/cleared in correct order
  • weak refs were cleared in Py_tp_clear iso. Py_tp_dealloc
  • some cursor members were not visited

This PR fixes these issues.

https://bugs.python.org/issue42972

@erlend-aasland
Copy link
Contributor Author

Whenever you have time, Pablo & Victor.

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 29, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 62bbb5c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 29, 2021
@vstinner
Copy link
Member

PPC64LE Fedora Stable Refleaks PR: 2:59:32 load avg: 1.00 running: test_asyncio (2 hour 39 min), unrelated issue.

buildbot/AMD64 Windows8.1 Refleaks PR:

0:25:45 load avg: 4.38 [ 44/427/1] test_concurrent_futures failed (20 min 5 sec) -- running: test_cmd_line (25 min 44 sec), test_tools (44.7 sec), test_marshal (20 min 22 sec)
beginning 6 repetitions
123456
.....Warning -- threading_cleanup() failed to cleanup 2 threads (count: 2, dangling: 3)
Warning -- Dangling thread: <Thread(ThreadPoolExecutor-254_0, started 3616)>
Warning -- Dangling thread: <Thread(ThreadPoolExecutor-254_1, started 2112)>
Warning -- Dangling thread: <_MainThread(MainThread, started 5044)>
test test_concurrent_futures failed -- Traceback (most recent call last):
  File "D:\buildarea\pull_request.ware-win81-release.refleak\build\lib\test\test_concurrent_futures.py", line 905, in test_idle_thread_reuse
    self.assertEqual(len(executor._threads), 1)
AssertionError: 2 != 1


test_asyncio

Warning -- Unraisable exception
Traceback (most recent call last):
  File "D:\buildarea\pull_request.ware-win81-release.refleak\build\lib\asyncio\windows_events.py", line 434, in select
    self._poll(timeout)
RuntimeError: <_overlapped.Overlapped object at 0x00000022989E54E0> still has pending operation at deallocation, the process may crash


4 re-run tests:
    test_asyncio test_concurrent_futures test_ssl test_threading
Total duration: 3 hour 2 min
Tests result: FAILURE then SUCCESS

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I checked that all Python objects are visited in traverse functions, and that clear functions clear all objects visited in traverse functions.

The coding style issue is not worth to require another revision.

@@ -42,9 +42,9 @@ row_clear(pysqlite_Row *self)
static int
row_traverse(pysqlite_Row *self, visitproc visit, void *arg)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: row.c is the only file where clear is declared before traverse.

@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 31, 2021
(cherry picked from commit d1124b0)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot
Copy link

GH-26461 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 31, 2021
@vstinner
Copy link
Member

@erlend-aasland: While reviewing traverse/clear functions, I noticed surprising code in pysqlite_cache_dealloc():

    if (!self->factory) {
        /* constructor failed, just get out of here */
        return;
    }

PyObject_GC_UnTrack() is not called if pysqlite_cache_init() failed, IMO it's a bug: the object is tracked in new, not in init.

Note: pysqlite_cache_init() doesn't use Py_SETREF() or Py_CLEAR() and so leaks references if called manually twice. Well, nobody should do that ;-)

@erlend-aasland erlend-aasland deleted the fixup-sqlite-gc branch May 31, 2021 08:46
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 31, 2021

While reviewing traverse/clear functions, I noticed surprising code in pysqlite_cache_dealloc():

    if (!self->factory) {
        /* constructor failed, just get out of here */
        return;
    }

PyObject_GC_UnTrack() is not called if pysqlite_cache_init() failed, IMO it's a bug: the object is tracked in new, not in init.

Note: pysqlite_cache_init() doesn't use Py_SETREF() or Py_CLEAR() and so leaks references if called manually twice. Well, nobody should do that ;-)

Yes, those are unfortunate issues :) However, they'll soon (hopefully) begone! Hint: #24203 😃 (You're welcome to take a look at that, if you find time)

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit d1124b0.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/292) and take a look at the build logs.
  4. Check if the failure is related to this commit (d1124b0) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/292

Failed tests:

  • test_idle

Failed subtests:

  • test_incremental_editing - idlelib.idle_test.test_colorizer.ColorDelegatorTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

412 tests OK.

1 test failed:
test_idle

14 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_ctypes
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_startfile test_winconsoleio test_winreg test_winsound
test_zipfile64

1 re-run test:
test_idle

Total duration: 26 min 43 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/tkinter_testing_utils.py", line 54, in new_test_method
    raise exception
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/tkinter_testing_utils.py", line 38, in after_callback
    next(test_generator)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/test_colorizer.py", line 573, in test_incremental_editing
    eq(text.tag_nextrange('BUILTIN', '1.0'), ('1.0', '1.3'))
AssertionError: Tuples differ: () != ('1.0', '1.3')


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/tkinter_testing_utils.py", line 54, in new_test_method
    raise exception
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/tkinter_testing_utils.py", line 38, in after_callback
    next(test_generator)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.11/idlelib/idle_test/test_colorizer.py", line 569, in test_incremental_editing
    eq(text.tag_nextrange('KEYWORD', '1.0'), ('1.0', '1.2'))
AssertionError: Tuples differ: () != ('1.0', '1.2')

vstinner pushed a commit that referenced this pull request May 31, 2021
(cherry picked from commit d1124b0)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing, @vstinner !

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.

6 participants