FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-117657: fix race:sock_recv_impl suppressions for free-thread building by Zheaoli · Pull Request #123697 · python/cpython · GitHub

/ cpython Public

gh-117657: fix race:sock_recv_impl suppressions for free-thread building - #123697

Merged
colesbury merged 6 commits into
python:mainfrom
Zheaoli:manjusaka/fix-socket-tsan
Sep 6, 2024
Merged

gh-117657: fix race:sock_recv_impl suppressions for free-thread building#123697
colesbury merged 6 commits into
python:mainfrom
Zheaoli:manjusaka/fix-socket-tsan

Conversation

Zheaoli commented Sep 4, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

… building

Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>

mpage commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

LGTM!

mpage requested review from DinoV and colesbury September 4, 2024 21:48
Zheaoli changed the title gh-123695: fix race:sock_recv_impl suppressions for free-thread building gh-117657: fix race:sock_recv_impl suppressions for free-thread building Sep 5, 2024

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think you diagnosed the issue correctly, but this changes the test so that it no longer actually tests the intended behavior of close().

Here is the (migrated) bug report that led to this test:

The point is to test that conn.close() actually closes the underlying socket, so that a recv/select on the other end (self.cli) with a short timeout returns immediately with an empty buffer.

  • Restore the deleted conn.close()
  • Move the select/recv checks to the other thread (_testClose) so that each thread only accesses their own end of the connection.

We might need a few more tweaks after that.

Copy link
Copy Markdown
Contributor

Something like:

    def testClose(self):
        conn, addr = self.serv.accept()
        conn.close()

        # Calling close() many times should be safe.
        conn.close()
        conn.close()

    def _testClose(self):
        self.cli.connect((HOST, self.port))
        read, write, err = select.select([self.cli], [], [], support.SHORT_TIMEOUT)
        self.assertEqual(read, [self.cli])
        self.assertEqual(self.cli.recv(1), b'')

        # The other end should be closed now, so select should immediately
        # return with the socket ready for reading.
        read, write, err = select.select([self.cli], [], [], 0)
        self.assertEqual(read, [self.cli])
        self.assertEqual(self.cli.recv(1), b'')

Zheaoli commented Sep 5, 2024

Copy link
Copy Markdown
Contributor Author

I think you diagnosed the issue correctly, but this changes the test so that it no longer actually tests the intended behavior of close().

Here is the (migrated) bug report that led to this test:

The point is to test that conn.close() actually closes the underlying socket, so that a recv/select on the other end (self.cli) with a short timeout returns immediately with an empty buffer.

  • Restore the deleted conn.close()
  • Move the select/recv checks to the other thread (_testClose) so that each thread only accesses their own end of the connection.

We might need a few more tweaks after that.

Thanks for the tips, I'll update the patch ASAP

Signed-off-by: Manjusaka <me@manjusaka.me>
Zheaoli requested a review from colesbury September 6, 2024 17:03

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This version of the PR has a few issues. The time.sleep(1.0) adds an unnecessary one second delay. The 0.1 timeout will make the test too sensitive to timing variations and lead to spurious failures on heavily loaded machines.

Please see the suggested code in my comment at: #123697 (comment)

Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>

Zheaoli commented Sep 6, 2024

Copy link
Copy Markdown
Contributor Author

This version of the PR has a few issues. The time.sleep(1.0) adds an unnecessary one second delay. The 0.1 timeout will make the test too sensitive to timing variations and lead to spurious failures on heavily loaded machines.

Please see the suggested code in my comment at: #123697 (comment)

Thanks for the review. I have updated this PR. I think this version of this PR is similar to your suggested code. I add an extra step to check the socket which is generated bt accept call works fine before the close action. I'm not sure this is necessary but I think this should be better.

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

colesbury self-assigned this Sep 6, 2024
colesbury merged commit 8a46a2e into python:main Sep 6, 2024
Zheaoli deleted the manjusaka/fix-socket-tsan branch September 7, 2024 08:42
colesbury removed their assignment Feb 14, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make TSAN tests pass with the GIL disabled in free-threaded builds

4 participants


Back | FazBrowse Home | New Git URL