Skip to content

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.

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
Merged
carljm merged 3 commits into from
Mar 6, 2024
Merged

gh-116143: Fix race condition in pydoc _start_server #116144

carljm merged 3 commits into from
Mar 6, 2024

Conversation

@itamaro
Copy link
Contributor

@itamaro itamaro commented Feb 29, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Feb 29, 2024
Closed
Jason-Y-Z
Jason-Y-Z reviewed Mar 2, 2024
View reviewed changes
Copy link
Contributor

@Jason-Y-Z Jason-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
Contributor Author

itamaro commented Mar 3, 2024

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 existing test_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!

carljm
carljm approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@carljm carljm 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 like thread.serving will always be set before thread.docserver. It's set in the thread.ready() callback, which is called in DocServer.server_activate, which is called from TCPServer.__init__, so it will definitely happen -- barring an error occurring instead -- before the DocServer(...) 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.

@carljm
Copy link
Member

carljm commented Mar 4, 2024

Oh -- I don't agree with the use of the skip 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.