Skip to content

Refactor association establishment and fix #589#677

Merged
Enet4 merged 6 commits intoEnet4:masterfrom
naterichman:refactor-association
Oct 1, 2025
Merged

Refactor association establishment and fix #589#677
Enet4 merged 6 commits intoEnet4:masterfrom
naterichman:refactor-association

Conversation

@naterichman
Copy link
Contributor

A lot of the logic in association establishment for both client/server is repeated in the sync/async methods. This PR attempts to extract any non-io logic into separate methods that can be shared between those methods.

In addition it also reduces duplication in

  • Sending/receiving PDUs
  • Error management for Server/Client

And finally, it adds tests that reproduce and confirm the fix for 589.

Full list of changes

* Move sans-io logic to their own respective functions
* Use shared logic for both sync and async implementations
* Bugfix for Enet4#589
* Further refactoring for as much code sharing as possible
* Testing for Enet4#589 regression
* Error names changed, so need to update into other workspace crates
@Enet4 Enet4 added bug This is a bug A-lib Area: library C-ul Crate: dicom-ul A-tool Area: tooling C-storescu Crate: dicom-storescu C-findscu Crate: dicom-findscu C-scpproxy Crate: dicom-scproxy C-storescp Crate: dicom-storescp labels Aug 26, 2025
@Enet4 Enet4 self-requested a review August 26, 2025 08:02
@naterichman naterichman mentioned this pull request Sep 15, 2025
@Enet4 Enet4 added the breaking change Hint that this may require a major version bump on release label Sep 17, 2025
@Enet4 Enet4 added this to the DICOM-rs 0.9 milestone Sep 17, 2025
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

The small API changes in dicom_ul are acceptable. The new tests are sound and they are all passing. Building and running the various DIMSE tools they seem to be working as intended. And the rest of the code changes look OK at a few glances.

Great! With this one merged it should now be easier to wrap up for 0.9.0. Thanks once again! 👍

@Enet4 Enet4 merged commit 7269f3e into Enet4:master Oct 1, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-lib Area: library A-tool Area: tooling breaking change Hint that this may require a major version bump on release bug This is a bug C-findscu Crate: dicom-findscu C-scpproxy Crate: dicom-scproxy C-storescp Crate: dicom-storescp C-storescu Crate: dicom-storescu C-ul Crate: dicom-ul

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream loss in association establishment when association acceptor sends more PDUs in quick succession

2 participants