Backport Thread Sanitizer infrastructure?

Hi!

In Add support for thread sanitizer (TSAN) via `--with-thread-sanitizer` · Issue #112536 · python/cpython · GitHub we started adding Thread Sanitizer (or “TSan” in short) builds on GitHub Actions, along with the required infrastructure in the codebase and test suite.

While the original motivation is to exercise the more sophisticated synchronization in the free-threaded builds, TSan can still be useful with the GIL enabled as the codebase has a number of non-trivial pieces of thread synchronization: for example, for pending calls execution, management of thread states, thread joining, interpreter finalization.

As a matter of fact, executing a couple of tests with TSan enabled has already found a number of synchronization issues despite the GIL:

The TSan-related additions are not bug fixes per se, but it might still be useful to backport them, so as to also add TSan-enabled builds on bugfix branches. For the most part, the TSan-related additions are in the test suite, but a number of them have to do with the core interpreter and build process:

  • addition of a _Py_THREAD_SANITIZER preprocessor variable
  • addition of a --with-thread-sanitizer configure option

Do you think we should try backporting those changes and add TSan-enabled builds to bugfix branches?

6 Likes

IMO, Since it will not affect the feature or performance itself, I think that it is okay to backport the thread sanitizer into 3.12 and 3.11

I’ve opened two initial backports PRs: [3.12] gh-112536: Add support for thread sanitizer (TSAN) (gh-112648) by pitrou · Pull Request #116924 · python/cpython · GitHub and [3.11] gh-112536: Add support for thread sanitizer (TSAN) (gh-112648) by pitrou · Pull Request #116926 · python/cpython · GitHub .

I’ll wait a bit for more feedback in this discussion. As I understand it, PyCon US is happening currently and people may not be very reactive.

@gpshead

PyCon US is in May :slight_smile:

3 Likes

Oh, well, looks like I misread :slight_smile:

2 Likes

These PRs don’t include the GitHub Actions changes to test thread sanitiser on the CI. Should they?

Our GitHub Actions is getting bigger so we need to keep an eye on resources. We’re hitting an Apple-shaped bottleneck at the moment, but these jobs use Ubuntu and are relatively quick: about 9 minutes for free-threaded and 6 minutes for regular (compared to ~26 mins for Windows); and we’d only be backporting the regular one, so should be fine.

1 Like

That will be done in followup backports, I think.

Does Microsoft / GH allocate us an increased amount of resources or are we using the regular open source offer?

We’re on the regular, free plan, with 20 total concurrent jobs and 5 max concurrent macOS jobs.

I’ve emailed a DevRel at GitHub to ask if they can help out, but in the meantime we’ve removed some less-essential macOS testing: python/cpython#116814.

1 Like

Speaking as RM for 3.12, given the scope of the proposed changes and that they’re a new build-time opt-in, I’m fine with them going in. (I’d be a little more wary if it turns out we need changes to the runtime to accommodate a ThreadSanitizer run, but that doesn’t seem very likely at this point.)

6 Likes

Ok, we now have a TSan-enabled Github Actions build on 3.12.
For 3.11, given that it will soon stop receiving bug fixes, I’m inclined not to bother after all.

4 Likes

I don’t think there’s much value in running TSan on 3.12. The data races we uncover are probably not worth addressing in 3.12 – I think the fixes are likely to be more risky than leaving most of the 3.12 data races as they are.

No, but it prevents any further regression.

That seems fine, but I think it means that we either:

  • Add suppressions in 3.12 for all the existing data races
  • Or limit the --tsan tests in 3.12 so that they don’t report the existing races, such as the ones you mention above
1 Like

For now we’ve added the same set of suppressions and of enabled tests as in git main.
(see details in [3.12] gh-112536: Add --tsan test for reasonable TSAN execution times. (gh-116601) by pitrou · Pull Request #116929 · python/cpython · GitHub)

1 Like

possible to backport this to 3.10 as well ? even interpreter itself may not benefit from tsan,
it can help c extension development. I’m developing a c extension, and it’s impossible to load tsan build extension without interpreter support