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

[Zheaoli]

Fix #117657

• Issue: tsan: fix race:sock_recv_impl suppressions for free-thread building #123695

• Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

[mpage]

LGTM!

[colesbury]

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:

• socket close() hangs client if used without prior shutdown() #43559

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.

[colesbury]

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]

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:

• socket close() hangs client if used without prior shutdown() #43559

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

[colesbury]

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)

[Zheaoli]

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]

LGTM