Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Association Request Timout Question/Feature #1284

Closed
BobSter3000 opened this issue Dec 8, 2021 · 5 comments · Fixed by #1285
Closed

Association Request Timout Question/Feature #1284

BobSter3000 opened this issue Dec 8, 2021 · 5 comments · Fixed by #1285

Comments

@BobSter3000
Copy link
Contributor

I am labeling this as a feature request because it just doesn't seem like a bug to me.

Currently in DicomClient.ClientOptions I can set the AssociationRequestTimeoutInMs. When using SendAsync with no cancellation token and the timeout expires the connection closes, but another connection/association is attempted because of the requests in the send queue. This just loops forever.

I could use a the SendAsync with a cancellation token that has a set timout and that does work, but the problem is that if this is set to low the client will close the connection waiting for a move response. This is really troublsome if there are no pending move responses sent back, only the final move response.

What is, according to you, missing from fo-dicom? Please describe.
If the Association Timeout actually closes the connection and flushes the pending requests queue.

Describe the solution you'd like
In DicomClientRequestAssociationState change

        if (winner == associationRequestTimesOut)
        {
            _dicomClient.Logger.Warn($"[{this}] Association request timeout");
            return await _dicomClient.TransitionToCompletedState(_initialisationParameters, cancellation).ConfigureAwait(false);
        }

to something like this

        if (winner == associationRequestTimesOut)
        {
            _dicomClient.Logger.Warn($"[{this}] Association request timeout");
            var exception = new DicomAssociationRequestTimedoutException();  // this is just an example it can be whatever
            return await _dicomClient.TransitionToCompletedWithErrorState(_initialisationParameters, exception, cancellation).ConfigureAwait(false);
        }

Something like this still allows the cancellation token timeout to be much higher and not having to wait for the longer timeout in the association request.

I have done a bit of testing with this and do not really see any issues, although I know there is extensive work currently going on with the DicomClient. Thoughts?

If this is acceptable I can submit a Pull Request.

@BobSter3000 BobSter3000 added the new label Dec 8, 2021
@BobSter3000 BobSter3000 changed the title Association Request Timout Association Request Timout Question/Feature Dec 8, 2021
@amoerie
Copy link
Collaborator

amoerie commented Dec 8, 2021

The current implementation of the association request timeout timeout system is mostly designed for connections that need some time to "initialize", such as long distance VPN.

While I don't object to your proposal, I am interested to hear your scenario. Because it means that the initial connection succeeds, but the association request times out?

@amoerie
Copy link
Collaborator

amoerie commented Dec 8, 2021

And yes, the DicomClient as it exists today will be gone entirely once the advanced client PR is merged, but this proposal can be applied there easily as well. In fact, with the advanced client you would have been able to implement this desired behavior yourself building on the public APIs only.

@amoerie amoerie added enhancement and removed new labels Dec 8, 2021
@BobSter3000
Copy link
Contributor Author

@amoerie Right, the association request is sent but the response may get lost, or not responded too. So the client would wait for the response, until the cancellation token if there was one. If not it will just sit there. With my proposal the Association Request timeout would act similar to the cancellation token.

@amoerie
Copy link
Collaborator

amoerie commented Dec 8, 2021

If you're up for it you're definitely welcome to make a PR. Depending on what gets merged first, an extra change will need to occur for the advanced DICOM client but I imagine that will be trivial.

@BobSter3000
Copy link
Contributor Author

Will do. Thanks.

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

Successfully merging a pull request may close this issue.

2 participants