Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-44914: Add tests for some invariants of tp_version_tag #27774

Merged
merged 13 commits into from
Aug 16, 2021
Merged

bpo-44914: Add tests for some invariants of tp_version_tag #27774

merged 13 commits into from
Aug 16, 2021

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Aug 15, 2021 •
edited by bedevere-bot
Loading

@markshannon
Copy link
Member

markshannon commented Aug 16, 2021 •
edited
Loading

Could you remove the changes to Include/internal/pycore_object.h and Objects/typeobject.c?

Now that #27773 is merged, the tests should pass.

@Fidget-Spinner Fidget-Spinner changed the title WIP: Do not reset tp_version_tag bpo-44914: Add tests for some invariants of tp_version_tag Aug 16, 2021
@Fidget-Spinner Fidget-Spinner marked this pull request as ready for review August 16, 2021 12:33
@Fidget-Spinner Fidget-Spinner marked this pull request as draft August 16, 2021 14:17
@Fidget-Spinner Fidget-Spinner marked this pull request as ready for review August 16, 2021 15:16
@Fidget-Spinner
Copy link
Member Author

FYI: these tests are designed to run on normal debug builds. So they will fail the normal CI to save us the pain of having to catch this with buildbots.

@markshannon
Copy link
Member

I think I preferred sys,_get_type_version_tag approach.
We aren't just testing the C API here, sys._clear_type_cache is visible to Python code.

What might be worth having is a testcapi function to set the next version tag. Then we can test what happens when version tag overflows. No need to do that for this PR, though.

@Fidget-Spinner
Copy link
Member Author

Sure. I threw it into testcapi because that's what the ma_version_tag tests used, but I'm fine with both sides for/against placing it in sys.

vstinner
vstinner reviewed Aug 16, 2021
View reviewed changes
Python/sysmodule.c Outdated Show resolved Hide resolved
Lib/test/test_type_cache.py Outdated Show resolved Hide resolved
Lib/test/test_type_cache.py Outdated Show resolved Hide resolved
Python/sysmodule.c Outdated Show resolved Hide resolved
Python/sysmodule.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

@markshannon: "I think I preferred sys,_get_type_version_tag approach."

Ah, you prefer to add a private function in the sys module? What would be the usage outside unit tests? If it's only for testing/debugging purpose, IMO _testcapi is more appropriate.

@markshannon
Copy link
Member

Looks good. Thanks.

@markshannon markshannon merged commit d84d2c4 into python:main Aug 16, 2021
@Fidget-Spinner Fidget-Spinner deleted the fix_tp_version_tag branch August 17, 2021 14:34
@vstinner
Copy link
Member

When you close/reopen a PR, can you please mention which tests of which CI failed please? If some tests are failing randomly, they should be fixed. Everyone is annoyed by such annoying tests.

@vstinner
Copy link
Member

On the merged commit, I see that test_tools_script_run_tests() of test_regrtest failed on Windows (x64). This CI is no longer mandatory?

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Aug 17, 2021 •
edited
Loading

When you close/reopen a PR, can you please mention which tests of which CI failed please? If some tests are failing randomly, they should be fixed. Everyone is annoyed by such annoying tests.

Got it, I'll do so in the future. I didn't open a bug on bpo this time because it's the first time I've seen this test fail like this

0:21:19 load avg: 0.02 [428/428/3] test_regrtest crashed (Exit code 1)
Timeout (0:20:00)!
Thread 0x00000d2c (most recent call first):
  File "D:\a\cpython\cpython\lib\subprocess.py", line 1493 in _readerthread
  File "D:\a\cpython\cpython\lib\threading.py", line 946 in run
  File "D:\a\cpython\cpython\lib\threading.py", line 1009 in _bootstrap_inner
  File "D:\a\cpython\cpython\lib\threading.py", line 966 in _bootstrap

so I thought it's just a rare one-off event (unlike test_asyncio, test_concurrent_futures etc.). If I see it a second time, I'll open a bug.

@Fidget-Spinner
Copy link
Member Author

On the merged commit, I see that test_tools_script_run_tests() of test_regrtest failed on Windows (x64). This CI is no longer mandatory?

Sorry, could you please give me the link? I'm not sure which one you're referring to. I see the Windows buildbot is green, and so is the GH actions

@vstinner
Copy link
Member

When I check on [Show details] on the merged commit line, I see that Windows (x64) failed:
https://github.com/python/cpython/runs/3342514542

@vstinner
Copy link
Member

I created:

The test_asyncio failure is known: https://bugs.python.org/issue41682#msg399770

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request Mar 2, 2023
@swtaarrs @facebook-github-bot
Make type version tags globally unique
39aab72 
Summary:
This is a backport of python/cpython#27773 and
python/cpython#27774 from CPython 3.12. They allow us
to perform lookups that assign a new valid version tag to a type in callbacks
invoked by `PyType_Lookup()`, which I need to do in D42512608 (and probably
others in the future).

I moved the call to `_PyClassLoader_ClearCache()` from `_PyType_ClearCache()`
to `type_cache_clear()` to ensure that it's called by both `_PyType_Fini()` and
`PyType_ClearCache()` (the upstream changes made it so `_PyType_Fini()` no
longer calls `_PyType_ClearCache()`, instead calling `type_cache_clear()`
directly).

Original Summaries:

bpo-44914: Maintain invariants of type version tags. (GH-27773)

* Do not invalidate type versions unnecessarily.

----

bpo-44914: Add tests for some invariants of tp_version_tag (GH-27774)

Reviewed By: carljm

Differential Revision: D42760412

fbshipit-source-id: 9d5f2a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@markshannon markshannon markshannon left review comments

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

Successfully merging this pull request may close these issues.
5 participants
@Fidget-Spinner @markshannon @vstinner @the-knights-who-say-ni @bedevere-bot