Skip to content

[ul] Implement the DIMSE Association State Machine - #781

Draft
pgimeno4d wants to merge 16 commits into
Enet4:masterfrom
pgimeno4d:implement-state-machine
Draft

pgimeno4d wants to merge 16 commits into
Enet4:masterfrom
pgimeno4d:implement-state-machine

Conversation

@pgimeno4d

@pgimeno4d pgimeno4d commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

This PR implements the behaviour of the DIMSE Association State Machine as defined in PS3.8 (2025d) section 9.2.3. Due to the complexity of the task, it's divided into multiple stages:

  • Cleanups and refactors (4 commits)
  • For sync operation:
    • Implement handling of state Sta13 from the state machine
    • Implement correct behaviour for abort()
    • Implement correct behaviour for release()
    • Implement correct behaviour for association establishment (establish_*())
    • Implement correct behaviour for send() and receive()
    • Maybe change the socket to be an Option for consistency with the async version (see below)
  • For async operation:
    • Implement handling of state Sta13 from the state machine
    • Change the socket to be an Option so that it can be dropped to close the connection, as tokio doesn't provide means to shut down both sides of the stream.
    • Implement correct behaviour for abort()
    • Implement correct behaviour for release()
    • Implement correct behaviour for association establishment (establish_*())
    • Implement correct behaviour for send() and receive()
  • Fix unit tests
  • Discuss send() parameter type and receive() Ok return value
  • Discuss send() and receive() error return values; decide what to do about send() when it returns SendTooLongPdu
  • Discuss what to do about PDataReader when the incoming PDU is not a P-DATA one
  • Add finalization_timeout option to tools, along the lines of [tools] Add connection timeout options via CLI flags to DIMSE Tools #792
  • Add new tests

This is intended to address #714; along the way it fixes ERR_7 from #768. It already addresses a minor concern I expressed in #728 (comment).

This is not yet ready for review. In particular the API (the type of the payload) for send() and receive() needs to be discussed, and some of the changes may be controversial, like the new possibility of a panic in send_pdata() and receive_pdata().

So, as discussed in #714 (comment) (point 3), the idea is to make send() and receive() deal with P-DATA-TF PDUs only. There are several ways to accomplish this:

  • Make send() accept a generic Pdu as now, but return an error if the variant passed is not Pdu::PData, and make receive() only able to return Pdu::PData. This would be the most backwards-compatible change, but given the big changes in the API, it seems like a more type-restricted approach would be in order.
  • Make a new type, e.g. PDataTf, and make Pdu::PData be Pdu::PData(PDataTf) instead of Pdu::PData { data: Vec<PDataValue> } as it is now. send() and receive() would accept/return PDataTf. How PDataTf is defined may be subject to discussion; I'd say Vec<PDataValue>, but due to the presence of a data field in Pdu::PData, someone may argue that struct PDataTf { data: Vec<PDataValue> } is preferable.
  • Just pass/return Vec<PDataValue>.

Opinions on the overall approach taken in this PR, as well as on the decision for send() and receive(), would be appreciated.

AI disclosure: This PR contains no AI-generated code.

@pgimeno4d

Copy link
Copy Markdown
Contributor Author

Hm, maybe CI can be disabled until it's ready for review?

@pgimeno4d
pgimeno4d force-pushed the implement-state-machine branch 3 times, most recently from 5172412 to 3e8bf37 Compare June 9, 2026 07:19
@pgimeno4d

Copy link
Copy Markdown
Contributor Author

Rebased, applied some minor changes (mostly comments) and completed the async versions of establish_*, release and abort; only send and receive are pending. Feedback would be appreciated on the API for them, and also on whether it's worth to convert the sync socket to be an Option or if it's better for it to stay as is.

I've also been taking a look at PDataReader and noticed something. It currently says:

            match msg {
                Pdu::PData { data } => {
                    // ... (process it)
                }
                _ => {
                    return Err(std::io::Error::new(
                        std::io::ErrorKind::UnexpectedEof,
                        "Unexpected PDU type",
                    ));
                }
            }

Same goes for the async version. That's pretty worrying as there's no way to process incoming association control messages at that point, making it impossible to implement the state machine behaviour, and in particular to handle association release or abort, in applications that use receive_pdata or any other form of PDataReader. The reader just returns an error and the PDU is lost. Something should be made about it.

PDataWriter, on the other hand, doesn't seem to be a concern, since it's only able to send P-DATA-TF PDUs.

pgimeno4d added 16 commits June 29, 2026 12:32
According to the comments, this was a kludge related to the now removed `Drop` traits.
The association_promiscuous tests were failing because they were set up so that the server association stopped responding after establishing the association.
This change allows to actually "close the transport connection" as required by the DICOM standard even for async connections, despite Tokio not implementing a `shutdown` method that allows shutting down both directions.
@pgimeno4d
pgimeno4d force-pushed the implement-state-machine branch from 85ef974 to 3cb35e0 Compare June 29, 2026 10:57
@pgimeno4d

Copy link
Copy Markdown
Contributor Author

Rebased and some cleanups applied. This can't go further without discussing the issues. I've added to the OP some checkboxes with the points left to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant