Skip to content

gh-105587: Remove assertion from _PyStaticObject_CheckRefcnt #105638

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

Conversation

eduardo-elizondo
Copy link
Contributor

@eduardo-elizondo eduardo-elizondo commented Jun 10, 2023

Currently, we are using _PyStaticObject_CheckRefcnt to check the reference count of objects during runtime shutdown. However, this check can be incorrect if an Extension mutates the immortal reference count. Since we can't guarantee that this can't ever happen, let's instead just remove this check.

@ghost
Copy link

ghost commented Jun 10, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@eduardo-elizondo eduardo-elizondo force-pushed the remove_static_object_fini_check branch from 5862de0 to bcdfa80 Compare June 10, 2023 21:38
@eduardo-elizondo
Copy link
Contributor Author

cc @ericsnowcurrently @kumaraditya303 let me know what you think!

@eduardo-elizondo
Copy link
Contributor Author

@sunmy2019 suggested to do a soft warning instead, which should be an easy change. So we can either keep the change as is, or go for a warning instead - I could go either just let me know what folks think!

@ericsnowcurrently
Copy link
Member

Making it a warning is a good idea.

@eduardo-elizondo
Copy link
Contributor Author

Sounds like a plan, fix coming up later today!

@kumaraditya303
Copy link
Contributor

SGTM, I'll review.

@eduardo-elizondo
Copy link
Contributor Author

@kumaraditya303 ready, this should do the trick

@ericsnowcurrently
Copy link
Member

Would it make sense to emit a Warning?

@eduardo-elizondo
Copy link
Contributor Author

Would it make sense to emit a Warning?

@ericsnowcurrently as in a Python Warning right (i.e: PyErr_WarnEx)? If so, that's what I was originally thinking but I had two conerns:

  1. I couldn't find a single case of raising a python warning inside pylifecycle.c (implicit convention?).
  2. These immortal checks happen pretty late in the runtime shutdown process that we don't even have access to the python exception machinery. That is, _PyStaticObjects_CheckRefcnt is called after _PyExc_Fini so that's why I stuck to basic stderr logging. We could in theory move the place where we do these checks in, but I don't think it's that big of a deal if we do a python warning vs stderr since we are in the runtime shutdown process anyways.

Thoughts?

@ericsnowcurrently
Copy link
Member

Both of your points are good ones. I retract my idea. 😄

@kumaraditya303 kumaraditya303 added the needs backport to 3.12 only security fixes label Jun 14, 2023
@kumaraditya303 kumaraditya303 changed the title gh-105587: Remove the use of _PyStaticObject_CheckRefcnt gh-105587: Remove assertion from _PyStaticObject_CheckRefcnt Jun 14, 2023
@kumaraditya303 kumaraditya303 merged commit 6199fe3 into python:main Jun 14, 2023
@miss-islington
Copy link
Contributor

Thanks @eduardo-elizondo for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 14, 2023
@bedevere-bot
Copy link

GH-105769 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 14, 2023
kumaraditya303 pushed a commit that referenced this pull request Jun 14, 2023
…GH-105638) (#105769)

gh-105587: Remove assertion from `_PyStaticObject_CheckRefcnt` (GH-105638)
(cherry picked from commit 6199fe3)

Co-authored-by: Eddie Elizondo <[email protected]>
@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 Windows10 3.12 has failed when building commit 0a9346d.

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/1159/builds/109) and take a look at the build logs.
  4. Check if the failure is related to this commit (0a9346d) 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/1159/builds/109

Failed tests:

  • test_concurrent_futures

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

== Tests result: FAILURE then SUCCESS ==

431 tests OK.

10 slowest tests:

  • test_math: 6 min
  • test_multiprocessing_spawn: 4 min 48 sec
  • test_wmi: 3 min 51 sec
  • test_unparse: 2 min 36 sec
  • test_lib2to3: 2 min 9 sec
  • test_capi: 2 min 9 sec
  • test_tokenize: 1 min 56 sec
  • test_compileall: 1 min 45 sec
  • test_mmap: 1 min 28 sec
  • test_io: 1 min 15 sec

36 tests skipped:
test.test_asyncio.test_unix_events test_curses test_dbm_gnu
test_dbm_ndbm test_devpoll test_epoll test_fcntl test_fork1
test_gdb test_grp test_ioctl test_kqueue test_multiprocessing_fork
test_multiprocessing_forkserver test_nis test_openpty
test_ossaudiodev test_peg_generator test_perf_profiler
test_perfmaps test_pipes test_poll test_posix test_pty test_pwd
test_readline test_resource test_spwd test_syslog
test_threadsignals test_wait3 test_wait4 test_xxlimited
test_xxtestfuzz test_zipfile64 test_zoneinfo

1 re-run test:
test_concurrent_futures

Total duration: 23 min 29 sec

Click to see traceback logs
Traceback (most recent call last):
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\runpy.py", line 198, in _run_module_as_main
    return _run_code(code, main_globals, None,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\runpy.py", line 88, in _run_code
    exec(code, run_globals)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\__main__.py", line 2, in <module>
    main()
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line 802, in main
    Regrtest().main(tests=tests, **kwargs)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line 732, in main
    with os_helper.temp_cwd(test_cwd, quiet=True):
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\contextlib.py", line 155, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 531, in temp_cwd
    with temp_dir(path=name, quiet=quiet) as temp_path:
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\contextlib.py", line 155, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 485, in temp_dir
    rmtree(path)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 442, in rmtree
    _rmtree(path)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 385, in _rmtree
    _waitfor(_rmtree_inner, path, waitall=True)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 330, in _waitfor
    func(pathname)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 382, in _rmtree_inner
    _force_run(fullname, os.rmdir, fullname)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\__init__.py", line 218, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.12.bolen-windows10\\build\\build\\test_python_5804�\\test_python_worker_8452�'


Traceback (most recent call last):
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 480, in temp_dir
    yield path
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line 533, in temp_cwd
    yield cwd_dir
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line 738, in main
    self._main(tests, kwargs)
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line 797, in _main
    sys.exit(0)
SystemExit: 0


Traceback (most recent call last):
  File "D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\__init__.py", line 207, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.12.bolen-windows10\\build\\build\\test_python_5804�\\test_python_worker_8452�'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants