gh-116143: Fix race condition in pydoc _start_server

[itamaro]

• Issue: Race condition in pydoc._start_server #116143

[Jason-Y-Z]

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

[itamaro]

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]

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]

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.

[Jason-Y-Z]

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

[itamaro]

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.

Added!

[miss-islington-app]

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

[bedevere-app]

GH-116415 is a backport of this pull request to the 3.12 branch.

[bedevere-app]

GH-116416 is a backport of this pull request to the 3.11 branch.