Fix SubprocessServer cache thread-safety and test isolation#38501
Conversation
d67d90f to
394428c
Compare
6e75846 to
7625f49
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses intermittent log warnings related to subprocess management in the Apache Beam Python SDK. By improving thread-safety in the SubprocessServer cache and enhancing test isolation for JavaJarExpansionServiceTest, the changes prevent global state contamination and ensure more reliable test execution. Additionally, diagnostic logging has been introduced to better monitor subprocess lifecycle events. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces debugging instrumentation to subprocess_server.py and associated tests, including internal buffers to track subprocess registration and purging, and enables global CLI logging in pytest.ini. It also refactors _next_id to rely on caller-side locking. The reviewer feedback highlights that the debugging buffers and hardcoded logic should be removed to prevent memory leaks and unnecessary overhead in production, and suggests reverting the logging changes to avoid excessive noise.
7625f49 to
e7e5881
Compare
e7e5881 to
2a4b762
Compare
There was a problem hiding this comment.
Code Review
This pull request improves test isolation in JavaJarExpansionServiceTest by using mock.patch and refactors the locking mechanism in SubprocessServer to ensure atomicity during registration. Feedback highlights that removing the lock from _next_id makes the method thread-unsafe if called independently, suggesting the use of a re-entrant lock or inlining the logic. Additionally, there is a concern regarding the thread safety of patching global singletons when tests are run in parallel.
|
The failure of |
|
r: @derrickaw |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
I have seen the following intermittent log warnings in some test workflow logs, which seems to be double purge or some problems with server shutdown.
beam/sdks/python/apache_beam/utils/subprocess_server.py
Lines 95 to 96 in 4c4a2c1
After some manual investigation (AI tooling not helpful here), however, I have revealed it is a global cache state that was being contaminated during tests.
Specifically,
JavaJarExpansionServiceTest.setUp()was clearing the global_live_owners list. This caused any owners existed in the list to be lost from the registry, leading to the warning message when those owners get purged.This PR fixes SubprocessServer cache thread safety and test isolation
mock.patch.objectinJavaJarExpansionServiceTest.setUp()to prevent contaminating the global_live_ownerscache for other tests._SharedCache.register()thread-safe.