GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42972: Fix sqlite3 traverse/clear by erlend-aasland · Pull Request #26452 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 29, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner May 29, 2021 18:29
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 29, 2021
@bedevere-bot bedevere-bot added the awaiting review label May 29, 2021
@erlend-aasland
erlend-aasland requested review from pablogsal and vstinner May 29, 2021 18:29
@erlend-aasland

erlend-aasland commented May 29, 2021

Copy link
Copy Markdown
Contributor Author

Whenever you have time, Pablo & Victor.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland added needs backport to 3.10 skip news labels May 29, 2021
@erlend-aasland
erlend-aasland force-pushed the fixup-sqlite-gc branch from bce1100 to 2e7f768 Compare May 29, 2021 18:42
Erlend E. Aasland
bpo-42972: Fix sqlite3 traverse/clear
62bbb5c
@erlend-aasland
erlend-aasland force-pushed the fixup-sqlite-gc branch from 2e7f768 to 62bbb5c Compare May 29, 2021 18:46
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 29, 2021
@bedevere-bot

bedevere-bot commented May 29, 2021

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

vstinner commented May 31, 2021

Copy link
Copy Markdown
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
  • test_threading BrokenBarrierError: https://bugs.python.org/issue11871
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner approved these changes May 31, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_sqlite/row.c
@@ -42,9 +42,9 @@ row_clear(pysqlite_Row *self)
static int
row_traverse(pysqlite_Row *self, visitproc visit, void *arg)

@vstinner vstinner May 31, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 31, 2021
@vstinner
vstinner merged commit d1124b0 into python:main May 31, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label May 31, 2021
@miss-islington

miss-islington commented May 31, 2021

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 31, 2021
Erlend Egeberg Aasland @miss-islington
bpo-42972: Fix sqlite3 traverse/clear functions (pythonGH-26452)
d406d95
(cherry picked from commit d1124b0)

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

bedevere-bot commented May 31, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 label May 31, 2021
@vstinner

vstinner commented May 31, 2021

Copy link
Copy Markdown
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 ;-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

erlend-aasland commented May 31, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented May 31, 2021

Copy link
Copy Markdown

⚠️⚠️⚠️ 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')
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner pushed a commit that referenced this pull request May 31, 2021
@miss-islington Erlend Egeberg Aasland
bpo-42972: Fix sqlite3 traverse/clear functions (GH-26452) (GH-26461)
ff359d7
(cherry picked from commit d1124b0)

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

erlend-aasland commented May 31, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing, @vstinner !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal pablogsal mentioned this pull request Jul 18, 2021
[3.10] Correct the order of regen-abidump #27228
Closed
@erlend-aasland erlend-aasland mentioned this pull request Aug 20, 2021
bpo-24905: Support BLOB incremental I/O in sqlite module #271
Closed
@vstinner vstinner mentioned this pull request Jun 9, 2022
[C API] Heap types (PyType_FromSpec) must fully implement the GC protocol #87138
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@pablogsal pablogsal Awaiting requested review from pablogsal

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @bedevere-bot @vstinner @miss-islington @pablogsal @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.