Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

eduardo-elizondo
Copy link
Contributor

@eduardo-elizondoeduardo-elizondo commentedJun 10, 2023
edited by bedevere-bot
Loading

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.

zooba reacted with thumbs up emoji
@ghost
Copy link

ghost commentedJun 10, 2023
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@eduardo-elizondoeduardo-elizondoforce-pushed theremove_static_object_fini_check branch from5862de0 tobcdfa80CompareJune 10, 2023 21:38
@eduardo-elizondo
Copy link
ContributorAuthor

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

@eduardo-elizondo
Copy link
ContributorAuthor

@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!

sunmy2019 reacted with thumbs up emoji

@ericsnowcurrently
Copy link
Member

Making it a warning is a good idea.

eduardo-elizondo reacted with thumbs up emoji

@eduardo-elizondo
Copy link
ContributorAuthor

Sounds like a plan, fix coming up later today!

ericsnowcurrently reacted with thumbs up emoji

@kumaraditya303
Copy link
Contributor

SGTM, I'll review.

@eduardo-elizondo
Copy link
ContributorAuthor

@kumaraditya303 ready, this should do the trick

@ericsnowcurrently
Copy link
Member

Would it make sense to emit aWarning?

@eduardo-elizondo
Copy link
ContributorAuthor

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

@kumaraditya303kumaraditya303 added the needs backport to 3.12only security fixes labelJun 14, 2023
@kumaraditya303kumaraditya303 changed the titlegh-105587: Remove the use of _PyStaticObject_CheckRefcntgh-105587: Remove assertion from_PyStaticObject_CheckRefcntJun 14, 2023
@kumaraditya303kumaraditya303 merged commit6199fe3 intopython:mainJun 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 requestJun 14, 2023
…ythonGH-105638)(cherry picked from commit6199fe3)Co-authored-by: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
@bedevere-bot
Copy link

GH-105769 is a backport of this pull request to the3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelJun 14, 2023
kumaraditya303 pushed a commit that referenced this pull requestJun 14, 2023
…GH-105638) (#105769)gh-105587: Remove assertion from `_PyStaticObject_CheckRefcnt` (GH-105638)(cherry picked from commit6199fe3)Co-authored-by: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
@bedevere-bot
Copy link

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

Hi! The buildbotAMD64 Windows10 3.12 has failed when building commit0a9346d.

What do you need to do:

  1. Don't panic.
  2. Checkthe 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", line198, in_run_module_as_mainreturn _run_code(code, main_globals,None,^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"D:\buildarea\3.12.bolen-windows10\build\Lib\runpy.py", line88, in_run_codeexec(code, run_globals)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\__main__.py", line2, in<module>    main()  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line802, inmain    Regrtest().main(tests=tests,**kwargs)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line732, inmainwith os_helper.temp_cwd(test_cwd,quiet=True):  File"D:\buildarea\3.12.bolen-windows10\build\Lib\contextlib.py", line155, in__exit__self.gen.throw(value)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line531, intemp_cwdwith temp_dir(path=name,quiet=quiet)as temp_path:  File"D:\buildarea\3.12.bolen-windows10\build\Lib\contextlib.py", line155, in__exit__self.gen.throw(value)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line485, intemp_dir    rmtree(path)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line442, inrmtree    _rmtree(path)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line385, in_rmtree    _waitfor(_rmtree_inner, path,waitall=True)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line330, in_waitfor    func(pathname)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line382, in_rmtree_inner    _force_run(fullname, os.rmdir, fullname)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\__init__.py", line218, in_force_runreturn 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", line480, intemp_diryield path  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\os_helper.py", line533, intemp_cwdyield cwd_dir  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line738, inmainself._main(tests, kwargs)  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\libregrtest\main.py", line797, in_main    sys.exit(0)SystemExit:0Traceback (most recent call last):  File"D:\buildarea\3.12.bolen-windows10\build\Lib\test\support\__init__.py", line207, in_force_runreturn 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

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

Successfully merging this pull request may close these issues.

5 participants
@eduardo-elizondo@ericsnowcurrently@kumaraditya303@miss-islington@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp