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

Gracefully cancel move #5

Open
spaenigs opened this issue May 19, 2022 · 6 comments
Open

Gracefully cancel move #5

spaenigs opened this issue May 19, 2022 · 6 comments

Comments

@spaenigs
Copy link

spaenigs commented May 19, 2022

Is it possible to send a c_cancel, e.g., to gracefully interrupt the move of a series?

cnt = 1
async for instance_ds in local.retrieve(
        remote,
        query_res=QueryResult(QueryLevel.SERIES, data_sets=[query_dataset]),
        report=retrieve_report
):
   # process image
   cnt += 1
   if cnt == 15:
      # cancel move
@moloney
Copy link
Owner

moloney commented May 25, 2022

Thanks for this suggestion. Currently this isn't implemented.

The most general solution to always clean up (including if an exception occurred in the loop) would be adding yet another context manager to work around the fact that for loops don't get "closed". From an API standpoint I find this proliferation of context managers a bit annoying, but I guess unavoidable...

Of course exposing the DICOM c_cancel operation in some lower level way as you requested also seems to make sense.

@spaenigs
Copy link
Author

Thanks for your reply. Is the application of additional context managers already possible somehow? If yes, I would be very grateful if you could provide a minimal working example.

@moloney
Copy link
Owner

moloney commented May 30, 2022

No this isn't currently possible, but it is something I would like to have implemented soon. I think the best API would be to have the retrieve method start returning an object that is both iterable and a context manager.

In the mean time as a workaround, you could do an IMAGE level query and then pare down the QueryResult to only include the images you want before passing it to the retrieve method.

@moloney
Copy link
Owner

moloney commented Jun 9, 2022

I just created PR #8 which is an attempt to address this, please try it out. I want to try to figure out some tests before merging.

I forgot async generators do have an aclose method, and the code is now setup so it should handle the resulting GeneratorExit exception and send a C-CANCEL if needed. There is a generic aclosing context manager in the util module that allows you to do the context manager approach if preferred, which would look like:

async with aclosing(local.retrieve(remote, query_res=my_qr)) as ret:
    async for instance_ds in ret:
        if my_cond:
            break

@moloney
Copy link
Owner

moloney commented Jun 17, 2022

After some iterations, I finally think the code in that PR is converging on the correct behavior. Unfortunately some pynetdicom functions (e.g. send_c_find and send_c_move) will block the thread until some network timeouts occur, which can take a while. I opened an issue to see if this could be improved in pynetdicom. If not, the only alternative would be to ignore these blocked threads when exiting the context manager.

@moloney
Copy link
Owner

moloney commented Jul 29, 2022

This is largely addressed by PR #8 that is now merged, except for the potential for a long timeout (~3 minutes) during the cleanup. Whether or not you experience this timeout depends also on server behavior (i.e. does it close the connection on its end when it sees it). Improving this would really need to happen at the pynetdicom level, and my idea about just abandoning blocked threads doesn't work because we need them to release any network ports.

So given all that, the above work around (just retrieve the subset you want) is still the best option for now. I will leave this issue open to track the slow timeout problem.

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

No branches or pull requests

2 participants