gemini-code-assist[bot] commented on code in PR #38501: URL: https://github.com/apache/beam/pull/38501#discussion_r3253432206
########## sdks/python/apache_beam/transforms/external_test.py: ########## @@ -799,7 +799,13 @@ def test_implicit_builder_with_constructor_method(self): class JavaJarExpansionServiceTest(unittest.TestCase): def setUp(self): - SubprocessServer._cache._live_owners = set() + # Temporarily override _live_owners with an empty set for this test, + # preventing contamination of the process-wide global cache and avoiding + # side effects on other tests. + patcher = mock.patch.object( + SubprocessServer._cache, '_live_owners', new=set()) + patcher.start() + self.addCleanup(patcher.stop) Review Comment:  The use of `mock.patch.object` on a global singleton (`SubprocessServer._cache`) is not thread-safe. If tests are executed in parallel within the same process (e.g., using certain test runners), this could lead to race conditions where one test's mock overrides another's. While this is a significant improvement over the previous global state modification, consider if a more isolated approach (such as instance-based cache or a dedicated test isolation utility) is feasible for the future to avoid direct access to internal attributes. ########## sdks/python/apache_beam/utils/subprocess_server.py: ########## @@ -78,13 +78,14 @@ def __init__(self, constructor, destructor): self._counter = 0 def _next_id(self): - with self._lock: - self._counter += 1 - return self._counter + # Caller must hold self._lock. + self._counter += 1 + return self._counter Review Comment:  Removing the lock from `_next_id` makes this method thread-unsafe if called directly. While the added comment warns callers, this is a fragile pattern that relies on all internal callers correctly managing the lock. If this method is only used within `register`, consider inlining its logic to eliminate the unsafe helper. Alternatively, using a re-entrant lock (`threading.RLock`) for the class would allow `_next_id` to remain safe while being called from other locked methods like `register`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
