github-actions[bot] commented on code in PR #41275:
URL: https://github.com/apache/beam/pull/41275#discussion_r4560641396
##########
sdks/python/apache_beam/metrics/execution.py:
##########
@@ -257,32 +257,50 @@ def __init__(self, step_name):
self.lock = threading.Lock()
self.metrics = {} # type: Dict[_TypedMetricName, MetricCell]
+ def _get_typed_metric_name(self, cell_type, metric_name):
+ if not isinstance(metric_name, str):
+ try:
+ cache = metric_name._typed_metric_names
+ except AttributeError:
+ cache = metric_name._typed_metric_names = {}
Review Comment:
**🟠High**: Caching `_TypedMetricName(cell_type, metric_name)` in
`metric_name._typed_metric_names` introduces a reference cycle (`metric_name ->
_typed_metric_names -> _TypedMetricName -> metric_name`) that causes
`pickle.loads(pickle.dumps(metric_name))`,
`dill.loads(dill.dumps(metric_name))`, and `copy.deepcopy(metric_name)` (as
well as `MetricKey` / `MetricResult` containing `metric_name`) to fail with
`AttributeError: 'MetricName' object has no attribute 'name'` once
`metric_name` has been passed to `MetricsContainer.get_*`.
During unpickling or `deepcopy` of `metric_name`, Python allocates an
uninitialized `MetricName.__new__(MetricName)` and memoizes it before
reconstructing its `__dict__`. Reconstructing `_typed_metric_names` invokes
`_TypedMetricName.__reduce__() -> _TypedMetricName(cell_type, metric_name)`,
whose `__init__` calls `metric_name.fast_name()` before `metric_name.__dict__`
has been populated.
To fix this, define `__reduce__` on `MetricName` in
`sdks/python/apache_beam/metrics/metricbase.py` so the transient cache is not
serialized:
```python
def __reduce__(self):
return MetricName, (self.namespace, self.name, self.urn, self.labels)
```
--
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]
