Skip to content

Conversation

@copybara-service
Copy link

@copybara-service copybara-service bot commented Nov 11, 2025

Add py free-threaded support
Modernize testing (-> re-enable tests on Windows)

@copybara-service copybara-service bot force-pushed the test_830892275 branch 6 times, most recently from e89e64c to c3911ac Compare November 11, 2025 16:48
@copybara-service copybara-service bot force-pushed the test_830892275 branch 13 times, most recently from 2f66cfe to f95ea76 Compare November 12, 2025 12:46
@eustas eustas changed the title add py free-threaded support Add py free-threaded support Nov 12, 2025
@copybara-service copybara-service bot force-pushed the test_830892275 branch 3 times, most recently from f57fb4f to 134d504 Compare November 12, 2025 13:21
Modernize testing (-> re-enable tests on Windows)

PiperOrigin-RevId: 830892275
@eustas
Copy link
Collaborator

eustas commented Nov 12, 2025

@ngoldbaum I would be happy if you take another look. Thanks in advance.

@ngoldbaum
Copy link

I would be happy if you take another look. Thanks in advance.

What is your testing strategy? I don't see any existing tests using threading or concurrent.futures.ThreadPoolExecutor to exercise the critical sections you've added here in a multithreaded context. IMO you really need to add explicit multithreaded tests when dealing with critical sections, because there are a lot of caveats and situations where they might get suspended for one reason or another, leading to possible issues.

You could look at using pytest-run-parallel to run the existing unit tests in a multithreaded context. This will mostly shake out use of global state in the implementation of the extension or in the brotli library itself.

Given that the existing test suite makes heavy use of temporary files being written to disk with explicit names, I don't think it makes sense to focus on making the test suite thread-safe, so I probably wouldn't use pytest-run-parallel here. You'll get more more benefit from adding tests that explicitly share a Compressor or Decompressor between threads.

When I write tests like this, I try to come up with an operation that can be divided into many steps that can be distributed across a thread pool. You'd then write a worker function that processes a block of data using a shared mutable data structure and exercises as much of the API as possible.

Here's a test like this I wrote yesterday: https://github.com/ngoldbaum/preshed/blob/a2246dacd93de8b7ebe8c2665331b60d46111cf3/preshed/tests/test_multithreaded.py#L52-L88

If you want to be extra sure you're not triggering thread safety issues, you can run the multithreaded tests you end up with using a TSan instrumented build of CPython and brotli. See here for more details on using TSan to validate thread safety.

@lysnikolaou
Copy link

In addition to the issue @ngoldbaum mentioned, I think brotli_compressor_leave needs to be called in the early exits (e.g. https://github.com/google/brotli/pull/1386/files#diff-aad61296d0b60444ff0b6dfb1e3a70ad5e743825ba28252514f8a66f611ac809R578) as well, right? Otherwise, that'll prevent follow up calls from succeeding. Is that expected behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants