Skip to content
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.

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

gh-128277: use relaxed atomics for sock_fd #128304

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Dec 28, 2024

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks good after the brief look

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

I'm not sure it's worth doing these sorts of changes for file descriptors. We still have unfixable race conditions when sockets are closed concurrently (see #121544)

@colesbury
Copy link
Contributor

The PR is fairly straightforward, so it's fine with me if you want to merge it. I just don't think we should bother with this sort of thing in the future.

@kumaraditya303 kumaraditya303 merged commit 7c72c1f into python:main Dec 31, 2024
49 checks passed
@kumaraditya303 kumaraditya303 deleted the sock_fd branch December 31, 2024 06:20
@kumaraditya303
Copy link
Contributor Author

I'm not sure it's worth doing these sorts of changes for file descriptors. We still have unfixable race conditions when sockets are closed concurrently (see #121544)

I see, I did this just for asyncio because tsan was complaining while running the existing test suite and this fixes all of this AFAICS. If we aren't planning to fix this maybe we should add some tsan suppressions for these for the time being?

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Fedora Stable Clang 3.x has failed when building commit 7c72c1f.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/234/builds/6793) and take a look at the build logs.
  4. Check if the failure is related to this commit (7c72c1f) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/234/builds/6793

Failed tests:

  • test.test_asyncio.test_events
  • test_interpreters

Failed subtests:

  • test_create_connection_local_addr_nomatch_family - test.test_asyncio.test_events.EPollEventLoopTests.test_create_connection_local_addr_nomatch_family

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/test_asyncio/test_events.py", line 715, in test_create_connection_local_addr_nomatch_family
    with self.assertRaises(OSError):
         ~~~~~~~~~~~~~~~~~^^^^^^^^^
AssertionError: OSError not raised


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

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.

4 participants