Skip to content

fix: remove C-ECHO messages - #65

Merged
nickamzol merged 1 commit into
UMEssen:mainfrom
mbits-imaging:echo-removal
Sep 14, 2026
Merged

nickamzol merged 1 commit into
UMEssen:mainfrom
mbits-imaging:echo-removal

Conversation

@feliwir

@feliwir feliwir commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #64

This can be validated by performing a STOW-RS against a DCMTK storescp:
Before alternating C-ECHO & C-STORE:

D: ===================== INCOMING DIMSE MESSAGE ====================
D: Message Type                  : C-ECHO RQ
D: Presentation Context ID       : 1
D: Message ID                    : 1594
D: Data Set                      : none
D: ======================= END DIMSE MESSAGE =======================
D: DcmDataset::read() TransferSyntax="Little Endian Implicit"
I: Received Store Request
D: ===================== INCOMING DIMSE MESSAGE ====================
D: Message Type                  : C-STORE RQ
D: Presentation Context ID       : 1
D: Message ID                    : 1595
D: Affected SOP Class UID        : CTImageStorage
D: Affected SOP Instance UID     : 1.3.12.2.1107.5.1.3.24338.4.0.260602921291147
D: Data Set                      : present
D: Priority                      : medium
D: ======================= END DIMSE MESSAGE =======================
....

with this patch applied only C-STORE:

D: ===================== INCOMING DIMSE MESSAGE ====================
D: Message Type                  : C-STORE RQ
D: Presentation Context ID       : 1
D: Message ID                    : 797
D: Affected SOP Class UID        : CTImageStorage
D: Affected SOP Instance UID     : 1.3.12.2.1107.5.1.3.24338.4.0.260604289371126
D: Data Set                      : present
D: Priority                      : medium
D: ======================= END DIMSE MESSAGE =======================
D: DcmDataset::read() TransferSyntax="Little Endian Implicit"
D: DcmItem::checkAndUpdateVR() setting undefined VR of PixelData (7fe0,0010) to 'OW'
I: storing DICOM file: ./CT.1.3.12.2.1107.5.1.3.24338.4.0.260604289371126
W: DICOM file already exists, overwriting: ./CT.1.3.12.2.1107.5.1.3.24338.4.0.260604289371126
D: DcmFileFormat::checkMetaHeaderValue() Version of MetaHeader is ok: 0x0001
D: DcmFileFormat::checkMetaHeaderValue() use SOPClassUID [1.2.840.10008.5.1.4.1.1.2] from Dataset
D: DcmFileFormat::checkMetaHeaderValue() use SOPInstanceUID [1.2.826.0.1.3680043.2.473.2.3472463830.1692.1087912310.331] from Dataset
D: DcmFileFormat::checkMetaHeaderValue() use new TransferSyntaxUID [Little Endian Implicit] on writing following Dataset
D: DcmFileFormat::validateMetaInfo() found 8 Elements in DcmMetaInfo 'metinf'
D: DcmDataset::read() TransferSyntax="Little Endian Implicit"
I: Received Store Request
D: ===================== INCOMING DIMSE MESSAGE ====================
D: Message Type                  : C-STORE RQ
D: Presentation Context ID       : 1
D: Message ID                    : 798
D: Affected SOP Class UID        : CTImageStorage
D: Affected SOP Instance UID     : 1.3.12.2.1107.5.1.3.24338.4.0.260602921291147
D: Data Set                      : present
D: Priority                      : medium
D: ======================= END DIMSE MESSAGE =======================
....

This improves throughput / performance for every case for me about 2x. As an additional benefit it also prevents termination of associations in some cases, giving about 20x speedups

@feliwir
feliwir marked this pull request as ready for review September 10, 2026 13:11
@feliwir

feliwir commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@nickamzol can you take a look at this any time soon? Currently we are maintaining our own custom fork / docker images, since this is non-functional for some our customers

@nickamzol nickamzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not getting back to you sooner.

This looks like a good fix to me. C-ECHO during association recycling was a long-standing design wart that I’d wanted to get rid of for quite some time, but it exposed a bug with failed/partial message exchanges. You handled this via discard_on_err.

Thanks!

@nickamzol nickamzol added the backend:dimse This issue affects the DIMSE backend label Sep 14, 2026
@nickamzol nickamzol self-assigned this Sep 14, 2026
@nickamzol
nickamzol merged commit 3d40223 into UMEssen:main Sep 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:dimse This issue affects the DIMSE backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Association pool doesn't reuse associations

2 participants