github-actions[bot] commented on code in PR #40822:
URL: https://github.com/apache/beam/pull/40822#discussion_r4497402249
##########
sdks/python/apache_beam/utils/multi_process_shared.py:
##########
@@ -439,7 +442,19 @@ def unsafe_hard_delete(self):
to this object exist, or (b) you are ok with all existing references to
this object throwing strange errors when derefrenced.
"""
- self._get_manager().unsafe_hard_delete_singleton(self._tag)
+ address_file = os.path.join(self._path, self._tag) + ".address"
+ if (self._manager is None and
+ not _process_level_singleton_manager.has_singleton(self._tag) and
+ not os.path.exists(address_file)):
+ return
+ try:
+ self._get_manager().unsafe_hard_delete_singleton(self._tag)
Review Comment:
**🟡 Medium**: If `self._manager is None` and `address_file` exists at line
448, calling `self._get_manager()` can still inadvertently spawn a new server
process just to delete it in two cases:
1. **Stale `.address` file**: If the server process already died without
removing `address_file`, `connect_manager()` inside `_get_manager()` will fail
after retries, run `os.unlink(address_file)`, and then loop again in `while
self._manager is None:`, see `not os.path.exists(address_file)`, and call
`self._create_server(address_file)`.
2. **Concurrent deletion (TOCTOU)**: Line 448 checks
`os.path.exists(address_file)` without holding `_process_local_lock` /
`self._cross_process_lock`. If another caller finishes deleting `address_file`
before `_get_manager()` acquires the locks, `_get_manager()` will see `not
os.path.exists(address_file)` and spawn a new server.
Consider adding a `create_if_missing: bool = True` parameter to
`_get_manager()` and calling `self._get_manager(create_if_missing=False)` here
(returning `None` inside the lock when `address_file` does not exist and the
tag is not registered in `_process_level_singleton_manager`).
--
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]
