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-116143: Fix race condition in pydoc _start_server#116144

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

Merged
carljm merged 3 commits intopython:mainfromitamaro:gh-116143-pydoc-race
Mar 6, 2024

Conversation

@itamaro
Copy link
Contributor

@itamaroitamaro commentedFeb 29, 2024•
edited by bedevere-appbot
Loading

Copy link
Contributor

@Jason-Y-ZJason-Y-Z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is great, thanks! Would it be possible to add a test for this as well?

@itamaro
Copy link
ContributorAuthor

This is great, thanks! Would it be possible to add a test for this as well?

Thank you!

I'm not sure about a specific test for this scenario. The existingtest_pydoc.PydocServerTest.test_server test kinda covers it, but it triggers only in a very specific thread scheduling sequence. I think a test that would force that exact condition would be complex to write (potentially requiring a bunch of refactoring to allow mocking out the right things), and highly coupled to the internal implementation details.
I might be missing an obvious way to test this scenario, so please let me know if you have something in mind!

Copy link
Member

@carljmcarljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It looks likethread.serving will always be set beforethread.docserver. It's set in thethread.ready() callback, which is called inDocServer.server_activate, which is called fromTCPServer.__init__, so it will definitely happen -- barring an error occurring instead -- before theDocServer(...) initialization returns. So there's not a race condition in the attributes being set, the window is always there; the race is just a matter of whether we get a thread context switch such that_start_server catches that window and is able to return during it.

This change fixes the bug, and looks like the minimal effective fix. I agree that it looks impractical to write a test for this without a significant refactor.

itamaro reacted with thumbs up emoji
@carljm
Copy link
Member

Oh -- I don't agree with the use of theskip news label on this PR, though. Can you add a news entry? This is potentially a real user-facing issue, not just a tests issue; the fix should be recorded in the changelog.

@carljmcarljm removed the skip news labelMar 4, 2024
@Jason-Y-Z
Copy link
Contributor

I see, yes I think in that case maybe we can skip adding tests for this change. Thanks again!

itamaro reacted with thumbs up emoji

@itamaro
Copy link
ContributorAuthor

Oh -- I don't agree with the use of theskip news label on this PR, though. Can you add a news entry? This is potentially a real user-facing issue, not just a tests issue; the fix should be recorded in the changelog.

Added!

Jason-Y-Z reacted with thumbs up emoji

@carljmcarljm added needs backport to 3.11only security fixes needs backport to 3.12only security fixes labelsMar 6, 2024
@carljmcarljm merged commit02ee475 intopython:mainMar 6, 2024
@miss-islington-app
Copy link

Thanks@itamaro for the PR, and@carljm for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMar 6, 2024
…116144)(cherry picked from commit02ee475)Co-authored-by: Itamar Oren <itamarost@gmail.com>
@bedevere-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMar 6, 2024
…116144)(cherry picked from commit02ee475)Co-authored-by: Itamar Oren <itamarost@gmail.com>
@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelMar 6, 2024
@bedevere-app
Copy link

GH-116416 is a backport of this pull request to the3.11 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.11only security fixes labelMar 6, 2024
carljm pushed a commit that referenced this pull requestMar 6, 2024
… (#116415)gh-116143: Fix race condition in pydoc _start_server (GH-116144)(cherry picked from commit02ee475)Co-authored-by: Itamar Oren <itamarost@gmail.com>
@itamaroitamaro deleted the gh-116143-pydoc-race branchMarch 6, 2024 15:05
carljm pushed a commit that referenced this pull requestMar 6, 2024
… (#116416)gh-116143: Fix race condition in pydoc _start_server (GH-116144)(cherry picked from commit02ee475)Co-authored-by: Itamar Oren <itamarost@gmail.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@carljmcarljmcarljm approved these changes

+1 more reviewer

@Jason-Y-ZJason-Y-ZJason-Y-Z left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@itamaro@carljm@Jason-Y-Z
[8]ページ先頭
©2009-2025 Movatter.jp