Repository: pydicom/pydicom

Issue:
Weakref blocking Filedataset pickle after access
I am experiencing an issue in which a pydicom Filedataset object begins to produce weakref errors when attempting to pickle it after the first data access. This happens after other kinds of access as well (e.g., putting it into a list). I'd like to have the option to pickle DICOM header data just in case there is a non-JSON-friendly tag, but this error is preventing that from happening.

I would expect the Filedataset object to remain fully referenced and able to be pickled when no specific weak referencing action has been taken. 

Here is my code to reproduce this issue:
# Load a dicom header
test = dicom.dcmread(fileName, stop_before_pixels=True)
pickle.dumps(test)
# Produces valid pickle output

# Just print the test object without doing anything else to it
test 
pickle.dumps(test)
# Produces this error:
Traceback (most recent call last):
  File "<ipython-input-20-d490e0c8e88e>", line 1, in <module>
    pickle.dumps(test)
TypeError: can't pickle weakref objects

I'm testing this using Spyder in an Anaconda 3 environment on Windows. 

module       | version
------       | -------
platform     | Windows-10-10.0.19041-SP0
Python       | 3.7.9 (default, Aug 31 2020, 17:10:11) [MSC v.1916 64 bit (AMD64)]
pydicom      | 2.1.0
gdcm         | _module not found_
jpeg_ls      | _module not found_
numpy        | 1.19.2
PIL          | 8.0.1



Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=h1) Report
> Merging [#1280](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=desc) (74f52d4) into [master](https://codecov.io/gh/pydicom/pydicom/commit/1762b397a858f1379626bdf25eb2c008aca91dc4?el=desc) (1762b39) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pydicom/pydicom/pull/1280/graphs/tree.svg?width=650&height=150&src=pr&token=YciLV15Lwr)](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1280      +/-   ##
==========================================
+ Coverage   95.22%   95.23%   +0.01%     
==========================================
  Files          60       60              
  Lines        9501     9515      +14     
==========================================
+ Hits         9047     9062      +15     
+ Misses        454      453       -1     
```


| [Impacted Files](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pydicom/dataset.py](https://codecov.io/gh/pydicom/pydicom/pull/1280/diff?src=pr&el=tree#diff-cHlkaWNvbS9kYXRhc2V0LnB5) | `97.68% <100.00%> (+0.16%)` | :arrow_up: |
| [pydicom/sequence.py](https://codecov.io/gh/pydicom/pydicom/pull/1280/diff?src=pr&el=tree#diff-cHlkaWNvbS9zZXF1ZW5jZS5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=footer). Last update [1762b39...9f2895f](https://codecov.io/gh/pydicom/pydicom/pull/1280?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/doc/release_notes/v2.2.0.rst b/doc/release_notes/v2.2.0.rst
index 34aaf0a57..da423292d 100644
--- a/doc/release_notes/v2.2.0.rst
+++ b/doc/release_notes/v2.2.0.rst
@@ -24,4 +24,5 @@ Changes
 
 Fixes
 .....
-* 
+* Fixed pickling a :class:`~pydicom.dataset.Dataset` instance with sequences
+  after the sequence had been read (:issue:`1278`)
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index 5457dedfa..2946822dd 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -26,7 +26,7 @@ import os.path
 import re
 from types import ModuleType, TracebackType
 from typing import (
-    Generator, TYPE_CHECKING, Optional, Tuple, Union, List, ItemsView,
+    TYPE_CHECKING, Optional, Tuple, Union, List, Any, ItemsView,
     KeysView, Dict, ValuesView, Iterator, BinaryIO, AnyStr,
     Callable, TypeVar, Type, overload
 )
@@ -2365,6 +2365,18 @@ class Dataset(Dict[BaseTag, _DatasetValue]):
         return dump_handler(
             self.to_json_dict(bulk_data_threshold, bulk_data_element_handler))
 
+    def __getstate__(self) -> Dict[str, Any]:
+        # pickle cannot handle weakref - remove parent
+        d = self.__dict__.copy()
+        del d['parent']
+        return d
+
+    def __setstate__(self, state: Dict[str, Any]) -> None:
+        self.__dict__.update(state)
+        # re-add parent - it will be set to the parent dataset on demand
+        # if the dataset is in a sequence
+        self.__dict__['parent'] = None
+
     __repr__ = __str__
 
 
diff --git a/pydicom/sequence.py b/pydicom/sequence.py
index cee7fb4f4..3d98d9bd0 100644
--- a/pydicom/sequence.py
+++ b/pydicom/sequence.py
@@ -4,8 +4,8 @@
 Sequence is a list of pydicom Dataset objects.
 """
 from typing import (
-    Iterable, Optional, List, cast, Union, overload, MutableSequence
-)
+    Iterable, Optional, List, cast, Union, overload, MutableSequence,
+    Dict, Any)
 import weakref
 
 from pydicom.dataset import Dataset
@@ -149,3 +149,14 @@ class Sequence(MultiValue[Dataset]):
     def __repr__(self) -> str:  # type: ignore[override]
         """String representation of the Sequence."""
         return f"<{self.__class__.__name__}, length {len(self)}>"
+
+    def __getstate__(self) -> Dict[str, Any]:
+        # pickle cannot handle weakref - remove _parent
+        d = self.__dict__.copy()
+        del d['_parent']
+        return d
+
+    def __setstate__(self, state: Dict[str, Any]) -> None:
+        self.__dict__.update(state)
+        # re-add _parent - it will be set to the parent dataset on demand
+        self.__dict__['_parent'] = None


Test patch:
diff --git a/pydicom/tests/test_dataset.py b/pydicom/tests/test_dataset.py
index 5b2ffbd98..3fafafb4d 100644
--- a/pydicom/tests/test_dataset.py
+++ b/pydicom/tests/test_dataset.py
@@ -2,7 +2,8 @@
 """Unit tests for the pydicom.dataset module."""
 
 import copy
-import warnings
+import pickle
+import weakref
 
 import pytest
 
@@ -1730,19 +1731,37 @@ class TestFileDataset:
     def setup(self):
         self.test_file = get_testdata_file('CT_small.dcm')
 
-    def test_pickle(self):
+    def test_pickle_raw_data(self):
         ds = pydicom.dcmread(self.test_file)
-        import pickle
         s = pickle.dumps({'ds': ds})
         ds1 = pickle.loads(s)['ds']
         assert ds == ds1
         assert ds1.Modality == 'CT'
 
+    def test_pickle_data_elements(self):
+        ds = pydicom.dcmread(self.test_file)
+        assert ds.OtherPatientIDsSequence.parent == weakref.ref(ds)
+        for e in ds:
+            # make sure all data elements have been loaded
+            pass
+        s = pickle.dumps({'ds': ds})
+        ds1 = pickle.loads(s)['ds']
+        assert ds1.OtherPatientIDsSequence.parent == weakref.ref(ds)
+        assert ds == ds1
+
+    def test_pickle_nested_sequence(self):
+        ds = pydicom.dcmread(get_testdata_file("nested_priv_SQ.dcm"))
+        for e in ds:
+            # make sure all data elements have been loaded
+            pass
+        s = pickle.dumps({'ds': ds})
+        ds1 = pickle.loads(s)['ds']
+        assert ds == ds1
+
     def test_pickle_modified(self):
         """Test pickling a modified dataset."""
         ds = pydicom.dcmread(self.test_file)
         ds.PixelSpacing = [1.0, 1.0]
-        import pickle
         s = pickle.dumps({'ds': ds})
         ds1 = pickle.loads(s)['ds']
         assert ds == ds1