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

Second sketch for mechanism to cancel DcmSCU negotiateAssociation before connectionTimeout elapses #79

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jogerh
Copy link

@jogerh jogerh commented Apr 17, 2023

If a server is unavailable, the DcmSCU::negotiateAssociation function call will block for the duration of the connectionTimeout. In many cases this timeout may be long, 30-60 seconds to ensure that we can robustly establish associations against slow servers. This is a long time for a user to wait for a device to shut down.

In some use cases, for example for portable imaging devices, the ability to quickly shut down is important to reduce the risk of the user pulling the power cable before the system is completely shut down. Such devices typically have a high risk that servers are temporarily unavailable, for example if they are serving multiple purposes and have to be moved from lab to lab.

This pull request adds support for early cancellation of TCP connect attempts by introducing a TCP poll interval and a cancellation callback to DUL_ASSOCIATESERVICEPARAMETERS. The requestAssociationTCP function will call the cancellation callback at intervals specified by the polling interval and allows the caller to decide if the TCP connect attempt should be cancelled.

On the DcmSCU level, DcmSCU::negotiateAssociation is changed to take a cancelation token as an optional argument, and allows the client to implement its own cancellation mechanism.

This draft has some weaknesses:

  • The effective timeout can be longer than the connection timeout because the lowest granularity is the poll interval (should be possible to fix by modifying the poll interval in the last iteration.
  • There is no sanity check of the poll interval. It should be shorter than the ConnectTimeout
  • The new variables in DUL_ASSOCIATESERVICEPARAMETERS are not really related to associations, but rather to network. Should they be moved elsewhere?
  • Most programmers may not care about TCP cancellation, which is only needed in very particular use cases. Changing DcmSCU::negotiateAssociation by adding a cancellation token complicates the API for everyone. The advantage is that the lifetime requirements for the cancelation token becomes very clear (it has to outlive the call to negotiateAssociation). A commercial alternative to DCMTK recently introduced a similar callback-based approach to enable TCP cancellation.
  • The change adds complexity to requestAssociationTCP which is already very complex. Can we do such a change safely without refactoring the dulfsm.cc into a better shape?
  • The tests attempts to find an available unused port using an SCP, but does not take ownership of it. This may cause intermittently failing unit tests on a heavily loaded build node running many tests in parallel.

100029962 added 6 commits April 17, 2023 08:46
… for cancelation of TCP connect attempts

Here we extend the DUL_ASSOCIATESERVICEPARAMETERS struct with members for:
* A cancelation callback function that can be implemented by client to cancel ongoing TCP connect attempts
* A poll/select interval that is used to chop the poll/select timeout into shorter intervals to enable checking for cancelation.
This is implemented using a cancellation callback (not a cancellation flag).
The reason for using a cancellation callback function is that this relieves
the dul implementation from having to deal with potential data races related
to a cancellation flag. It is now up to the caller to ensure appropriate
synchronization of any data being involved in cancellation.
…s argument

We'll use this cancel token to cancel ongoing TCP connect attempts. A cancel token
allows the client to decide how a request should be cancelled, for example using
std::stop_token in the implementation of the ICancelToken.
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