gh-153935: Fix data race on socket timeout in the free-threaded build

[deadlovelll]

Fix data race on socket timeout in the free-threaded build

For more details see gh-153935

[ZeroIntensity]

These shouldn't be atomic on the GILicious build. Add a wrapper to pycore_pyatomic_ft_wrappers.h and use that here.

[deadlovelll]

Thanks for the review, I'll keep that in mind for the future. I've pushed the changes

[kumaraditya303]

FWIW I don't think it is worth adding tests for this, initially when I worked on fixing races on socket fd I didn't add tests because they won't be exhaustive and the race is trivial.

[deadlovelll]

hello, there's a tsan ci job that runs test_free_threading, so this test is a regression guard here, thats why i added it. but i'd be happy to remove it if you'd rather not have it

[nascheme]

Be careful when fixing TSAN warnings by “sprinkling” relaxed atomic accesses into the code. Relaxed atomics make accesses to that individual object race-free and provide a modification order for that object, but they do not establish a happens-before relationship or synchronize other state. They are appropriate when the value is logically independent, or when synchronization of related state is provided elsewhere. If the value publishes, guards, or must remain consistent with other memory locations—or with external state such as a file descriptor’s kernel mode—acquire/release ordering or, more often, a lock or another synchronization protocol is required.

Here, sock_timeout is not independent: its meaning depends on the blocking mode of the underlying file descriptor. Making only sock_timeout atomic removes the C data race reported by TSAN, but does not make updates to those two pieces of state atomic or mutually consistent. Acquire/release alone would not solve that transactional consistency problem.

[nascheme]

Here's my attempt at resolving this. In summary:

• hold a mutex while setting timeout/blocking. This ensures that multiple threads cannot be concurrently performing these operations
• set timeout using atomic release, this allows it to be read without the mutex, with an atomic acquire. It's possible relaxed would be sufficient here but release/acquire is safer.
• use a separate mutex rather than a critical section because the critical section would be dropped by Py_BEGIN_ALLOW_THREADS

We should also add a note to the documentation, something like:

   .. note::

      Do not call :meth:`setblocking` or :meth:`settimeout` while another
      thread is performing an operation on the same socket. Changing the
      blocking mode concurrently with operations such as :meth:`connect`,
      :meth:`accept`, :meth:`recv`, or :meth:`send` can cause the operation
      to block unexpectedly or to fail with a non-blocking error.

      Applications must use external synchronization when changing a socket's
      blocking mode or timeout. This also applies to socket objects and file
      descriptors that share the same underlying socket, since the blocking
      mode is shared at the operating-system level.

[nascheme]

BTW, if that's the rule, three of the new unit tests are violating it:

• test_operation_vs_settimeout
• test_sendall_vs_settimeout
• test_connect_vs_settimeout
  Probably we can just remove them because the other tests still check settimeout(), setblocking(), gettimeout() and getblocking(). That's allowed and would detect a data-race on the timeout member.