gh-124872: Replace enter/exit events with "switched" by Eclips4 · Pull Request #125532 · python/cpython

Eclips4 · 2024-10-15T15:12:33Z

Issue: test_capi leaks references #125512

📚 Documentation preview 📚: https://cpython-previews--125532.org.readthedocs.build/

Issue: Replace context enter and exit events with single "context switched" event #124872

Eclips4 · 2024-10-15T15:20:30Z

With that PR, test_capi is no longer leaking:

admin@Admins-MacBook-Air~/p/cpython (revert-of-revert)> ./python.exe-mtest-R3:3test_capiUsingrandomseed: 9920954540:00:00loadavg: 26.10Run1testsequentiallyinasingleprocess0:00:00loadavg: 26.10 [1/1] test_capibeginning6repetitions. Showingnumberofleaks (. for0orless, Xfor10ormore)
123:456XX. ...
test_capipassedin43.3sec==Testsresult: SUCCESS==1testOK.
Totalduration: 43.4secTotaltests: run=985skipped=67Totaltestfiles: run=1/1Result: SUCCESS

vstinner · 2024-10-15T15:25:34Z

gh-125512: Revert #125513 and fix reference cycle #125532

It's not a revert, but a revert of a revert :-D The PR title should be gh-124872: Replace enter/exit events with "switched.

vstinner · 2024-10-15T15:25:58Z

I suggest to wait after Python 3.14.0 alpha1 release to retry this change.

1st1 · 2024-10-15T15:47:23Z

I suggest to wait after Python 3.14.0 alpha1 release to retry this change.

Why? We can debug and fix the ref leak.

@rhansen Please build CPython with ./configure --with-pydebug and run the tests with -R3:3. That will take much longer so I suggest just running asyncio & contextvars & decimal tests with this flag.

vstinner · 2024-10-15T15:53:45Z

me:

I suggest to wait after Python 3.14.0 alpha1 release to retry this change.

@1st1:

Why?

It's just that there are many broken buildbots, I think that this change can wait for 3.14.0a2.

Eclips4 · 2024-10-15T16:22:08Z

FYI, I ran the test suite in hunt refleaks mode locally and it succeeded:

==Testsresult: SUCCESS==25testsskipped:
test.test_asyncio.test_windows_eventstest.test_asyncio.test_windows_utilstest.test_gdb.test_backtracetest.test_gdb.test_cfunctiontest.test_gdb.test_cfunction_fulltest.test_gdb.test_misctest.test_gdb.test_pretty_printtest.test_multiprocessing_fork.test_managertest.test_multiprocessing_fork.test_misctest.test_multiprocessing_fork.test_processestest.test_multiprocessing_fork.test_threadstest_androidtest_dbm_gnutest_devpolltest_epolltest_free_threadingtest_launchertest_msvcrttest_perf_profilertest_perfmapstest_startfiletest_winapitest_winconsoleiotest_winregtest_wmi11testsskipped (resourcedenied):
test_cursestest_peg_generatortest_pyrepltest_smtpnettest_socketservertest_tkintertest_ttktest_urllib2nettest_urllibnettest_winsoundtest_zipfile64442testsOK.
Totalduration: 20min5secTotaltests: run=44,347skipped=2,182Totaltestfiles: run=467/478skipped=25resource_denied=11Result: SUCCESS

1st1 · 2024-10-15T16:32:03Z

@Eclips4 thanks for fixing the leak!

@vstinner

It's just that there are many broken buildbots, I think that this change can wait for 3.14.0a2.

I sympathize, really sorry about breaking them and causing pain :/ Can we try landing this PR since it appears it should solve the issue? I'm afraid if we wait on this for a month to get it merged it will go stale and we just forget. I'd try again and if it fails we wait? If however you really don't want to do it, then fine.

ambv · 2024-10-15T16:35:56Z

Since we're about to release a1 tonight, I'd wait with landing this until right after a1. We won't forget about it.

ambv · 2024-10-15T16:36:55Z

Actually, @hugovk should make the call.

hugovk · 2024-10-15T16:49:41Z

Yeah, same call, thanks :)

rhansen

Apologies for the breakage, and thank you for debugging!

Is a blurb required now that there's an API change between v3.14.0a1 and v3.14.0a2?

Also, would you mind fixing up the commit message, authorship info, etc. on the first commit:

git -c sequence.editor='sed -i -e "1s/^pick/edit/"' rebase -i main &&
git commit --amend -C 843d28f59d2 &&
git rebase --continue

1st1 · 2024-10-16T00:02:13Z

@ambv @vstinner

Since we're about to release a1 tonight, I'd wait with landing this until right after a1. We won't forget about it.

Oh I was reading the schedule wrong. I didn't realize Victor wants to delay it just one day, essentially, I thought we were talking one month to merge this. Absolutely can wait a few days.

hugovk · 2024-10-16T05:24:00Z

Thanks, a1 is now out, merge whenever you're ready.

vstinner · 2024-10-16T09:33:53Z

I applied @rhansen's change, before that, the test did nothing :-)

Also, would you mind fixing up the commit message, authorship info, etc. on the first commit:

That sounds like a good idea :-) Or at least, make sure that @rhansen is mentioned as a co-author of the change.

Eclips4 · 2024-10-16T10:09:55Z

Apologies for the breakage, and thank you for debugging!
Is a blurb required now that there's an API change between v3.14.0a1 and v3.14.0a2?
Also, would you mind fixing up the commit message, authorship info, etc. on the first commit:
git -c sequence.editor='sed -i -e "1s/^pick/edit/"' rebase -i main &&
git commit --amend -C 843d28f59d2 &&
git rebase --continue

Sorry about that! I definitely will do it.

…4776) Users want to know when the current context switches to a different context object. Right now this happens when and only when a context is entered or exited, so the enter and exit events are synonymous with "switched". However, if the changes proposed for pythongh-99633 are implemented, the current context will also switch for reasons other than context enter or exit. Since users actually care about context switches and not enter or exit, replace the enter and exit events with a single switched event. The former exit event was emitted just before exiting the context. The new switched event is emitted after the context is exited to match the semantics users expect of an event with a past-tense name. If users need the ability to clean up before the switch takes effect, another event type can be added in the future. It is not added here because YAGNI. I skipped 0 in the enum as a matter of practice. Skipping 0 makes it easier to troubleshoot when code forgets to set zeroed memory, and it aligns with best practices for other tools (e.g., https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum).

Co-authored-by: Victor Stinner <vstinner@python.org>

Co-authored-by: Richard Hansen <rhansen@rhansen.org>

Eclips4 · 2024-10-16T10:20:37Z

@rhansen it seems I have done it. If you agree, I will merge it.

(I'm actually do not understand why for f0b3aa862b629690496d0336904a9bb1ada6dd47 github is treating me as co-author...)

vstinner · 2024-10-16T10:42:35Z

(I'm actually do not understand why for f0b3aa8 github is treating me as co-author...)

GitHub is correct when saying:

rhansen authored and Eclips4 committed

vstinner

LGTM

ZeroIntensity · 2024-10-16T11:35:25Z

Should this get tested with the refleak buildbots?

vstinner · 2024-10-16T11:45:47Z

Should this get tested with the refleak buildbots?

I don't think so. I tested test_capi manually, and @Eclips4 ran the whole test suite with -R 3:3.

…5532) Users want to know when the current context switches to a different context object. Right now this happens when and only when a context is entered or exited, so the enter and exit events are synonymous with "switched". However, if the changes proposed for pythongh-99633 are implemented, the current context will also switch for reasons other than context enter or exit. Since users actually care about context switches and not enter or exit, replace the enter and exit events with a single switched event. The former exit event was emitted just before exiting the context. The new switched event is emitted after the context is exited to match the semantics users expect of an event with a past-tense name. If users need the ability to clean up before the switch takes effect, another event type can be added in the future. It is not added here because YAGNI. I skipped 0 in the enum as a matter of practice. Skipping 0 makes it easier to troubleshoot when code forgets to set zeroed memory, and it aligns with best practices for other tools (e.g., https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum). Co-authored-by: Richard Hansen <rhansen@rhansen.org> Co-authored-by: Victor Stinner <vstinner@python.org>

Eclips4 requested review from 1st1 and ericsnowcurrently as code owners October 15, 2024 15:12

bedevere-appBot mentioned this pull request Oct 15, 2024
test_capi leaks references #125512
Closed

bedevere-appBot added the awaiting core review label Oct 15, 2024

Eclips4 added the skip news label Oct 15, 2024

Eclips4 requested a review from vstinner October 15, 2024 15:13

Eclips4 changed the title ~~gh-125512: Revert #125513 and fix reference cycle~~gh-124872: Replace enter/exit events with "switched"Oct 15, 2024

bedevere-appBot mentioned this pull request Oct 15, 2024
Replace context enter and exit events with single "context switched" event #124872
Open

rhansen suggested changes Oct 15, 2024
View reviewed changes

Comment threadLib/test/test_capi/test_watchers.py Outdated

rhansenand others added 3 commits October 16, 2024 13:17

Fix reference cycle
caa33e9
Co-authored-by: Victor Stinner <vstinner@python.org>

Update Lib/test/test_capi/test_watchers.py
049834c
Co-authored-by: Richard Hansen <rhansen@rhansen.org>

Eclips4 force-pushed the revert-of-revert branch from 436edd2 to 049834cCompare October 16, 2024 10:18

vstinner approved these changes Oct 16, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting core review labels Oct 16, 2024

Merge branch 'main' into revert-of-revert
b81dd6b

ambv merged commit bee112a into python:mainOct 16, 2024

bedevere-appBot removed the awaiting merge label Oct 16, 2024

Eclips4 deleted the revert-of-revert branch October 16, 2024 12:07

Uh oh!

Uh oh!

Conversation

Eclips4 commented Oct 15, 2024• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eclips4 commented Oct 15, 2024

Uh oh!

vstinner commented Oct 15, 2024

Uh oh!

vstinner commented Oct 15, 2024

Uh oh!

1st1 commented Oct 15, 2024

Uh oh!

vstinner commented Oct 15, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eclips4 commented Oct 15, 2024

Uh oh!

1st1 commented Oct 15, 2024

Uh oh!

ambv commented Oct 15, 2024

Uh oh!

ambv commented Oct 15, 2024

Uh oh!

hugovk commented Oct 15, 2024

Uh oh!

rhansen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1st1 commented Oct 16, 2024

Uh oh!

hugovk commented Oct 16, 2024

Uh oh!

vstinner commented Oct 16, 2024

Uh oh!

Eclips4 commented Oct 16, 2024

Uh oh!

Eclips4 commented Oct 16, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 16, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Oct 16, 2024

Uh oh!

vstinner commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Eclips4 commented Oct 15, 2024•
edited by bedevere-app Bot
Loading

vstinner commented Oct 15, 2024•
edited
Loading

Eclips4 commented Oct 16, 2024•
edited
Loading