Repository: pydicom/pynetdicom
Pull request #526 (2020-08-01 21:16:59)

[MRG] feat: write C-STORE-RQ dataset to disk in chunks
Extends #520 to SCP.

#### Reference issue
ref: #517 

#### Tasks
- [x] Unit tests added that reproduce issue or prove feature is working
- [x] Fix or feature added
- [x] Documentation and examples updated (if relevant)
- [x] Unit tests passing and coverage at 100% after adding fix/feature
- [x] Apps updated and tested (if relevant)


scaramallion: Sorry, been busy. I'll try and have a proper look soon
r1b: This is interesting, from Windows test output:

```
E: Exception in handler bound to 'evt.EVT_C_STORE'
E: [Errno 13] Permission denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmptwbnftvq.dcm'
Traceback (most recent call last):
  File "D:\a\pynetdicom\pynetdicom\pynetdicom\service_class.py", line 1446, in SCP
    rsp_status = evt.trigger(
  File "D:\a\pynetdicom\pynetdicom\pynetdicom\events.py", line 212, in trigger
    return handlers[0](evt)
  File "D:\a\pynetdicom\pynetdicom\pynetdicom\tests\test_service_storage.py", line 424, in handle
    ds = dcmread(dataset_path)
  File "c:\hostedtoolcache\windows\python\3.8.5\x64\lib\site-packages\pydicom\filereader.py", line 847, in dcmread
    fp = open(fp, 'rb')
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmptwbnftvq.dcm'
```

Maybe this will work if we use the default mode (w+b) when initially opening the file.
r1b: @scaramallion I'm marking this `MRG` since I don't think there are any remaining major blockers, though I'm happy to address some of the remaining comments above depending on your feedback
r1b: Ok figured out the Windows crash:

https://github.com/bravoserver/bravo/issues/111#issuecomment-826990
https://github.com/bravoserver/bravo/issues/111#issuecomment-838099
scaramallion: Very nice
r1b: Marking WIP again until I add docs

Patch:
diff --git a/docs/reference/config.rst b/docs/reference/config.rst
index 6cc4c47a5..83d5176a3 100644
--- a/docs/reference/config.rst
+++ b/docs/reference/config.rst
@@ -15,5 +15,6 @@ Configuration Options (:mod:`pynetdicom._config`)
    LOG_HANDLER_LEVEL
    LOG_REQUEST_IDENTIFIERS
    LOG_RESPONSE_IDENTIFIERS
+   STORE_RECV_CHUNKED_DATASET
    STORE_SEND_CHUNKED_DATASET
    USE_SHORT_DIMSE_AET
diff --git a/pynetdicom/_config.py b/pynetdicom/_config.py
index f51ad2750..9bd19b66f 100644
--- a/pynetdicom/_config.py
+++ b/pynetdicom/_config.py
@@ -111,9 +111,14 @@ If ``True``, then when using
 a DICOM dataset, don't decode the dataset and instead send the raw encoded
 data (without the File Meta Information) in chunks of no larger than
 maximum PDU size allowed by the peer. This should
-minimise the amount of memory required when sending large datasets, however
-no conversion of the dataset is possible and so an exact matching accepted
-presentation context will be required. Default: ``False``.
+minimise the amount of memory required when:
+
+* Sending large datasets
+* Sending many datasets concurrently
+
+It is not possible to convert the dataset without loading it into memory
+and so an exact matching accepted presentation context will be required.
+Default: ``False``.
 
 Examples
 --------
@@ -121,3 +126,27 @@ Examples
 >>> from pynetdicom import _config
 >>> _config.STORE_SEND_CHUNKED_DATASET = True
 """
+
+STORE_RECV_CHUNKED_DATASET = False
+"""Chunk a dataset file when receiving it to minimise memory usage.
+
+.. versionadded:: 2.0
+
+If ``True``, then when receiving C-STORE primitives as an SCP, don't encode
+the dataset and instead receive the raw data in chunks of no larger
+than the maximum PDU size allowed by the peer. The dataset is written to a
+temporary file on disk (with the File Meta Information). The path of the
+dataset is available in the C-STORE event handler on the property
+``dataset_path``. This should minimise the amount of memory required when:
+
+* Receiving large datasets
+* Receiving many datasets concurrently
+
+Default: ``False``.
+
+Examples
+--------
+
+>>> from pynetdicom import _config
+>>> _config.STORE_RECV_CHUNKED_DATASET = True
+"""
diff --git a/pynetdicom/_handlers.py b/pynetdicom/_handlers.py
index 7353cb3b4..4002dc693 100644
--- a/pynetdicom/_handlers.py
+++ b/pynetdicom/_handlers.py
@@ -781,7 +781,7 @@ def _send_c_store_rq(event):
     dataset = "None"
     if msg.data_set and msg.data_set.getvalue() != b"":
         dataset = "Present"
-    elif msg._data_set_file is not None:
+    elif msg._data_set_path is not None:
         dataset = "Present"
 
     if cs.AffectedSOPClassUID.name == "CT Image Storage":
diff --git a/pynetdicom/association.py b/pynetdicom/association.py
index 85ea3a749..a9b16169b 100644
--- a/pynetdicom/association.py
+++ b/pynetdicom/association.py
@@ -1731,7 +1731,7 @@ class Association(threading.Thread):
                 dataset = None
                 allow_conversion = False
                 file_meta, offset = split_dataset(fpath)
-                req._dataset_file = (fpath, offset)
+                req._dataset_path = (fpath, offset)
 
                 missing = [
                     'MediaStorageSOPClassUID',
diff --git a/pynetdicom/dimse.py b/pynetdicom/dimse.py
index f0d3114d7..a8f333908 100644
--- a/pynetdicom/dimse.py
+++ b/pynetdicom/dimse.py
@@ -244,7 +244,7 @@ class DIMSEServiceProvider(object):
         if self.message is None:
             self.message = DIMSEMessage()
 
-        if self.message.decode_msg(primitive):
+        if self.message.decode_msg(primitive, self.assoc):
             # Trigger event
             evt.trigger(
                 self.assoc, evt.EVT_DIMSE_RECV, {'message' : self.message}
diff --git a/pynetdicom/dimse_messages.py b/pynetdicom/dimse_messages.py
index f2d643570..c05043e6b 100644
--- a/pynetdicom/dimse_messages.py
+++ b/pynetdicom/dimse_messages.py
@@ -4,14 +4,18 @@ from __future__ import division
 from io import BytesIO
 import logging
 from math import ceil
+from pathlib import Path
+from tempfile import NamedTemporaryFile
 
 from pydicom.dataset import Dataset
+from pydicom.filewriter import write_file_meta_info
 
+from pynetdicom import _config
 from pynetdicom.dimse_primitives import (
     C_STORE, C_FIND, C_GET, C_MOVE, C_ECHO, C_CANCEL,
     N_EVENT_REPORT, N_GET, N_SET, N_ACTION, N_CREATE, N_DELETE
 )
-from pynetdicom.dsutils import encode, decode
+from pynetdicom.dsutils import encode, decode, create_file_meta
 from pynetdicom.pdu_primitives import P_DATA
 
 
@@ -214,8 +218,13 @@ class DIMSEMessage(object):
         self.data_set = BytesIO()
         self.command_set = Dataset()
 
-        # If reading the dataset in chunks this will be
-        #   (its file path, byte offset to the start of the dataset)
+        # If reading the dataset in chunks this will be a tuple:
+        #   (its file path, a byte offset to the start of the dataset)
+        # If writing the dataset in chunks this will be a Path:
+        #   its file path
+        self._data_set_path = None
+        # If writing the dataset in chunks this will be a NamedTemporaryFile:
+        #   the file object backing its file path
         self._data_set_file = None
 
         cls_name = self.__class__.__name__
@@ -226,7 +235,7 @@ class DIMSEMessage(object):
         for keyword in _COMMAND_SET_KEYWORDS[cls_name.replace('_', '-')]:
             setattr(self.command_set, keyword, None)
 
-    def decode_msg(self, primitive):
+    def decode_msg(self, primitive, assoc=None):
         """Converts P-DATA primitives into a ``DIMSEMessage`` sub-class.
 
         Decodes the data from the P-DATA service primitive (which
@@ -241,6 +250,17 @@ class DIMSEMessage(object):
         primitive : pdu_primitives.P_DATA
             The P-DATA service primitive to be decoded into a DIMSE message.
 
+        assoc : association.Association
+            The Association that produced the P-DATA service primitive. This
+            is only necessary when:
+
+            * :attr:`~pynetdicom._config.STORE_RECV_CHUNKED_DATASET` is ``True``
+            * The P_DATA primitive is a part of a C_STORE_RQ DIMSE message
+
+            In this case the Association is consulted for its accepted
+            transfer syntax, which is included in the File Meta Information
+            of the stored dataset.
+
         Returns
         -------
         bool
@@ -310,6 +330,33 @@ class DIMSEMessage(object):
                         #   has been completely decoded
                         return True
 
+                    if (
+                        _config.STORE_RECV_CHUNKED_DATASET and
+                        isinstance(self, C_STORE_RQ)
+                    ):
+                        # delete=False is a workaround for Windows
+                        # Setting delete=True prevents us from re-opening
+                        # the file after it is opened by NamedTemporaryFile
+                        # below.
+                        self._data_set_file = NamedTemporaryFile(
+                            delete=False,
+                            mode="wb",
+                            suffix=".dcm"
+                        )
+                        self._data_set_path = Path(self._data_set_file.name)
+
+                        self._data_set_file.write(b'\x00' * 128)
+                        self._data_set_file.write(b'DICM')
+
+                        write_file_meta_info(
+                            self._data_set_file,
+                            create_file_meta(
+                                sop_class_uid=self.command_set.AffectedSOPClassUID,
+                                sop_instance_uid=self.command_set.AffectedSOPInstanceUID,
+                                transfer_syntax=assoc._accepted_cx[context_id].transfer_syntax[0],
+                            )
+                        )
+
             # DATA SET
             # P-DATA fragment contains Data Set information
             #   (control_header_byte is xxxxxx00 or xxxxxx10)
@@ -317,7 +364,10 @@ class DIMSEMessage(object):
                 # As with the command set, the data set may be spread over
                 #   a number of fragments in each P-DATA primitive and a
                 #   number of P-DATA primitives.
-                self.data_set.write(data[1:])
+                if self._data_set_file:
+                    self._data_set_file.write(data[1:])
+                else:
+                    self.data_set.write(data[1:])
 
                 # The final data set fragment (xxxxxx10) has been added
                 if control_header_byte & 2 != 0:
@@ -433,12 +483,12 @@ class DIMSEMessage(object):
                     [context_id, b'\x02' + next(ds_fragments)]
                 )
                 yield pdata
-        elif self._data_set_file is not None:
+        elif self._data_set_path is not None:
             # Read and send encoded dataset from file
             # Buffer size determined by io.DEFAULT_BUFFER_SIZE
-            with open(self._data_set_file[0], 'rb') as f:
+            with open(self._data_set_path[0], 'rb') as f:
                 end = f.seek(0, 2)
-                length = end - f.seek(self._data_set_file[1])
+                length = end - f.seek(self._data_set_path[1])
 
                 if max_pdu_length == 0:
                     nr_fragments = 1
@@ -546,6 +596,9 @@ class DIMSEMessage(object):
         # Set the presentation context ID the message was set under
         primitive._context_id = self.context_id
 
+        primitive._dataset_path = self._data_set_path
+        primitive._dataset_file = self._data_set_file
+
         return primitive
 
     def primitive_to_message(self, primitive):
@@ -602,8 +655,8 @@ class DIMSEMessage(object):
             # 'C_CANCEL_RQ', 'N_DELETE_RSP', 'C_FIND_RSP', 'N_GET_RQ'
             pass
 
-        self._data_set_file = getattr(primitive, "_dataset_file", None)
-        if self._data_set_file:
+        self._data_set_path = getattr(primitive, "_dataset_path", None)
+        if self._data_set_path:
             self.command_set.CommandDataSetType = 0x0001
 
         # Set the Command Set length
diff --git a/pynetdicom/dimse_primitives.py b/pynetdicom/dimse_primitives.py
index 84096494e..5a12989d4 100644
--- a/pynetdicom/dimse_primitives.py
+++ b/pynetdicom/dimse_primitives.py
@@ -524,8 +524,19 @@ class C_STORE(DIMSEPrimitive):
         # For Failure statuses 0x0117
         # self.AffectedSOPInstanceUID
 
-        # If None then the dataset is encoded as BytesIO
-        # If not None then the dataset is stored at (path, offset)
+        # If we are sending a C-STORE service primitive:
+        #   If None then the dataset is encoded as BytesIO
+        #   If not None then the dataset is stored at (path, offset)
+        # If we are receiving a C-STORE service primitive:
+        #   If None then the dataset is encoded as BytesIO
+        #   If not None then the dataset is stored at _dataset_path
+        self._dataset_path = None
+        # If we are sending a C-STORE service primitive:
+        #   Always None
+        # If we are receiving a C-STORE service primitive:
+        #   If None then the dataset is encoded as BytesIO
+        #   If not None then _dataset_file backs the dataset stored
+        #   at _dataset_path
         self._dataset_file = None
 
     @property
diff --git a/pynetdicom/dsutils.py b/pynetdicom/dsutils.py
index 46a5d1c77..f3a77db1d 100644
--- a/pynetdicom/dsutils.py
+++ b/pynetdicom/dsutils.py
@@ -1,13 +1,14 @@
 """DICOM dataset utility functions."""
 
 import logging
-import os
 import zlib
 
+from pydicom import Dataset
 from pydicom.filebase import DicomBytesIO
 from pydicom.filereader import read_dataset, read_preamble
-from pydicom.filewriter import write_dataset, write_data_element
+from pydicom.filewriter import write_dataset
 
+from pynetdicom import PYNETDICOM_IMPLEMENTATION_UID, PYNETDICOM_IMPLEMENTATION_VERSION
 from pynetdicom.utils import pretty_bytes
 
 
@@ -243,3 +244,56 @@ def split_dataset(fpath):
             stop_when=_not_group_0002
         )
         return file_meta, fp.tell()
+
+
+def create_file_meta(
+    *,
+    sop_class_uid,
+    sop_instance_uid,
+    transfer_syntax,
+    group_length=0,
+    version=b'\x00\x01',
+    implementation_uid=PYNETDICOM_IMPLEMENTATION_UID,
+    implementation_version=PYNETDICOM_IMPLEMENTATION_VERSION,
+):
+    """Return a new file meta dataset
+
+    .. versionadded:: 2.0
+
+    Parameters
+    ----------
+    sop_class_uid : pydicom.uid.UID
+        The value for the MediaStorageSOPClassUID attribute.
+    sop_instance_uid : pydicom.uid.UID
+        The value for the MediaStorageSOPInstanceUID attribute.
+    transfer_syntax : pydicom.uid.UID
+        The value for the TransferSyntax attribute.
+    group_length : int
+        The value for the FileMetaInformationGroupLength attribute.
+    version : bytes
+        The value for the FileMetaInformationVersion attribute.
+    implementation_uid : pydicom.uid.UID
+        The value for the ImplementationClassUID attribute.
+    implementation_version : str
+        The value for the ImplementationVersionName attribute.
+
+    Returns
+    -------
+    pydicom.dataset.Dataset
+        The File Meta dataset
+    """
+    file_meta = Dataset()
+
+    file_meta.FileMetaInformationGroupLength = group_length
+    file_meta.FileMetaInformationVersion = version
+    file_meta.MediaStorageSOPClassUID = sop_class_uid
+    file_meta.MediaStorageSOPInstanceUID = sop_instance_uid
+    file_meta.TransferSyntaxUID = transfer_syntax
+    file_meta.ImplementationClassUID = implementation_uid
+    file_meta.ImplementationVersionName = implementation_version
+
+    # File Meta Information is always encoded as Explicit VR Little Endian
+    file_meta.is_little_endian = True
+    file_meta.is_implicit_VR = False
+
+    return file_meta
diff --git a/pynetdicom/events.py b/pynetdicom/events.py
index a0d518336..2c0fe7809 100644
--- a/pynetdicom/events.py
+++ b/pynetdicom/events.py
@@ -10,7 +10,7 @@ import sys
 
 from pydicom.dataset import Dataset
 
-from pynetdicom.dsutils import decode
+from pynetdicom.dsutils import decode, create_file_meta
 
 
 LOGGER = logging.getLogger('pynetdicom.events')
@@ -420,6 +420,19 @@ class Event(object):
         )
         return self._get_dataset("DataSet", msg)
 
+    @property
+    def dataset_path(self):
+        try:
+            dataset_path = self.request._dataset_path
+        except AttributeError:
+            msg = (
+                "The corresponding event is either not a C-STORE request or "
+                "'STORE_RECV_CHUNKED_DATASET' is not set."
+            )
+            raise AttributeError(msg)
+
+        return dataset_path
+
     @property
     def event(self):
         """Return the corresponding event.
@@ -549,25 +562,13 @@ class Event(object):
                 "The corresponding event is not a C-STORE request"
             )
 
-        from pynetdicom import PYNETDICOM_IMPLEMENTATION_UID
-        from pynetdicom import PYNETDICOM_IMPLEMENTATION_VERSION
-
         # A C-STORE request must have AffectedSOPClassUID and
         #   AffectedSOPInstanceUID
-        meta = Dataset()
-        meta.FileMetaInformationGroupLength = 0
-        meta.FileMetaInformationVersion = b'\x00\x01'
-        meta.MediaStorageSOPClassUID = self.request.AffectedSOPClassUID
-        meta.MediaStorageSOPInstanceUID = self.request.AffectedSOPInstanceUID
-        meta.TransferSyntaxUID = self.context.transfer_syntax
-        meta.ImplementationClassUID = PYNETDICOM_IMPLEMENTATION_UID
-        meta.ImplementationVersionName = PYNETDICOM_IMPLEMENTATION_VERSION
-
-        # File Meta Information is always encoded as Explicit VR Little Endian
-        meta.is_little_endian = True
-        meta.is_implicit_VR = False
-
-        return meta
+        return create_file_meta(
+            sop_class_uid=self.request.AffectedSOPClassUID,
+            sop_instance_uid=self.request.AffectedSOPInstanceUID,
+            transfer_syntax=self.context.transfer_syntax,
+        )
 
     def _get_dataset(self, attr, exc_msg):
         """Return DIMSE dataset-like parameter as a *pydicom* Dataset.
diff --git a/pynetdicom/service_class.py b/pynetdicom/service_class.py
index c8f1aeccb..1f58c1d7d 100644
--- a/pynetdicom/service_class.py
+++ b/pynetdicom/service_class.py
@@ -2,6 +2,7 @@
 
 from io import BytesIO
 import logging
+import os
 import sys
 import traceback
 
@@ -1448,6 +1449,16 @@ class StorageServiceClass(ServiceClass):
                 evt.EVT_C_STORE,
                 {'request' : req, 'context' : context.as_tuple,}
             )
+            if req._dataset_file:
+                req._dataset_file.close()
+
+                try:
+                    # We passed delete=False when creating the temporary file
+                    os.unlink(req._dataset_file.name)
+                except OSError:
+                    # This is best effort on e.g Windows where files may
+                    # not be deleted while in use.
+                    pass
 
         # Exception in context or handler aborted/released
         if not ctx.success or not self.assoc.is_established:


Test patch:
diff --git a/pynetdicom/tests/test_events.py b/pynetdicom/tests/test_events.py
index f260055c9..5dfa52a99 100644
--- a/pynetdicom/tests/test_events.py
+++ b/pynetdicom/tests/test_events.py
@@ -190,6 +190,13 @@ class TestEvent(object):
         with pytest.raises(AttributeError, match=msg):
             event.message_id
 
+        msg = (
+            r"The corresponding event is either not a C-STORE request or "
+            r"'STORE_RECV_CHUNKED_DATASET' is not set."
+        )
+        with pytest.raises(AttributeError, match=msg):
+            event.dataset_path
+
     def test_is_cancelled_non(self):
         """Test Event.is_cancelled with wrong event type."""
         event = evt.Event(None, evt.EVT_DATA_RECV)
diff --git a/pynetdicom/tests/test_service_storage.py b/pynetdicom/tests/test_service_storage.py
index ec9d68100..9c6375bc9 100644
--- a/pynetdicom/tests/test_service_storage.py
+++ b/pynetdicom/tests/test_service_storage.py
@@ -2,6 +2,7 @@
 
 from io import BytesIO
 import os
+from pathlib import Path
 import time
 
 import pytest
@@ -10,7 +11,7 @@ from pydicom import dcmread
 from pydicom.dataset import Dataset
 from pydicom.uid import ExplicitVRLittleEndian
 
-from pynetdicom import AE, evt, build_role, debug_logger
+from pynetdicom import AE, _config, evt, build_role, debug_logger
 from pynetdicom.dimse_primitives import C_STORE
 from pynetdicom.pdu_primitives import SOPClassExtendedNegotiation
 from pynetdicom.service_class import StorageServiceClass
@@ -42,6 +43,8 @@ class TestStorageServiceClass(object):
         if self.ae:
             self.ae.shutdown()
 
+        _config.STORE_RECV_CHUNKED_DATASET = False
+
     @pytest.mark.skipif(not HAS_STATUS, reason="No Status class available")
     def test_status_enum(self):
         """Test failure to decode the dataset"""
@@ -410,6 +413,85 @@ class TestStorageServiceClass(object):
 
         scp.shutdown()
 
+    def test_scp_handler_dataset_path(self):
+        """Test handler event's dataset_path property"""
+        attrs = {}
+
+        def handle(event):
+            dataset_path = event.dataset_path
+
+            # File at `dataset_path` valid and complete
+            ds = dcmread(dataset_path)
+            assert isinstance(ds, Dataset)
+            assert isinstance(ds.file_meta, Dataset)
+            assert ds.PatientName == DATASET.PatientName
+            assert (
+                ds.file_meta.MediaStorageSOPInstanceUID
+                == DATASET.file_meta.MediaStorageSOPInstanceUID
+            )
+
+            # `dataset` property empty
+            assert event.dataset == Dataset()
+
+            attrs['dataset_path'] = dataset_path
+            return 0x0000
+
+        _config.STORE_RECV_CHUNKED_DATASET = True
+
+        handlers = [(evt.EVT_C_STORE, handle)]
+
+        self.ae = ae = AE()
+        ae.add_supported_context(CTImageStorage)
+        ae.add_requested_context(CTImageStorage)
+        scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
+
+        assoc = ae.associate('localhost', 11112)
+        assert assoc.is_established
+        status = assoc.send_c_store(DATASET)
+        assert status.Status == 0x0000
+        assoc.release()
+        assert assoc.is_released
+
+        dataset_path = attrs['dataset_path']
+        assert isinstance(dataset_path, Path)
+
+        # `dataset_path` not available outside of event handler
+        with pytest.raises(FileNotFoundError):
+            dataset_path.open("rb")
+
+        scp.shutdown()
+
+    def test_scp_handler_dataset_path_windows_unlink(self, monkeypatch):
+        """Test handler event's dataset_path property:
+            user has file open on Windows"""
+
+        def unlink(*args, **kwargs):
+            raise OSError()
+
+        import os
+        monkeypatch.setattr(os, 'unlink', unlink)
+
+        def handle(event):
+            return 0x0000
+
+        _config.STORE_RECV_CHUNKED_DATASET = True
+
+        handlers = [(evt.EVT_C_STORE, handle)]
+
+        self.ae = ae = AE()
+        ae.add_supported_context(CTImageStorage)
+        ae.add_requested_context(CTImageStorage)
+        scp = ae.start_server(('', 11112), block=False, evt_handlers=handlers)
+
+        assoc = ae.associate('localhost', 11112)
+        assert assoc.is_established
+        status = assoc.send_c_store(DATASET)
+        assert status.Status == 0x0000
+        assoc.release()
+        assert assoc.is_released
+
+        scp.shutdown()
+
     def test_scp_handler_move_origin(self):
         """Test handler event's request property with MoveOriginator"""
         attrs = {}