Hi,

I found bug in Dicom.DicomClient, Dicom.DicomServer, and DicomServices.ScuBase - classes - when I made the Destructor and Dispose methods I copied and pasted with an error.  The current code is:

               GC.SuppressFinalize(true);

it should be:

               GC.SuppressFinalize(this);

Regards,

Dan

Dan,

Sorry for the delay in responding. I've made the updates in my local files, although I haven't committed it yet.

FYI, when we were integrating the Scus into the Viewer, we found a few threading issues in how Dispose and network shutdowns were handled. We believe these are fixed, but this raised a debate as to whether it actually made sense to have a Dispose() method for both the Scus and DicomClient/DicomServer. We basically came to the conclusion that the use of Dispose in this case probably doesn't make sense, and we should have an "Abort()" method instead to rapidly shut down a connection. A ticket was created for this here (https://trac.clearcanvas.ca/source/ticket/2465), although we obviously haven't done any work on it yet.

Steve

Hi Steve,

Can you give more details on the issues you were having and why you came to that conclusion?

Can't the ScuBase's Dispose() method call the newly Abort() method?

Because, the point of the dispose pattern is to ensure all un-managed resources are released, which network connections are.

Thanks,

Dan

Hi Dan,

I talked about it a bit more about this with Stewart this morning. The conclusion was not necessarily an aversion to the use of Dispose() per se, it was more that the current Dispose() method was not cleaning up associations properly according to DICOM, and it isn't entirely clear from the current API what the proper way to clean up an association was. You're right, we could call the Abort() method, if applicable, from Dispose(). We'll think this through a bit more before we implement the ticket to make sure we get it right.

Steve

Hi Steve,

I am looking into this now, because we can't force a connection to close - Cancel() doesn't really do much, and Dispose() does not dispose of the network connection immediately, we have to wait until the current store or operation is completed..

Anyway, reading over the thread - I agree we should make a proper Abort() method w/ at first trying to send a dicom associate abort - perhaps this can replace the existing Cancel() method.

What I believe is preventing a Scu to dispose/abort properly, is NetworkBase.ShutdownNetworkThread() - the _thread.Join() waits until the current dicom store is finished .. so, if I change it to say _thread.Join(2500), then it will close... although, could create threading issues like you mentioned before...

anyway, attached is a attempt at this.. note, in DicomClient.Abort() - probably want to do some checks before calling SendAssociateAbort() to make sure it is in a valid state to do it...

thanks,

dan

Dan,

We're at the tail end of a release cycle, and I'm a bit reluctant to make changes on this (we're still developing in the trunk). I did take a quick look at the patch, however. One thing to note, according to proper DICOM, when an Abort is sent, the side sending the abort is supposed to allow the side receiving the abort to drop the connection as a response to the abort. The best way to handle this would probably be to have a timeout passed to Abort (and also to Join), where optionally a user could wait for the proper shutdown, or else just put in a 1 or 2 second timeout, if they don't want to wait... In any case, I'll follow up on this in probably another week or two.

Steve