oops, first time submitting a patch, didn't realize there was a source contribution forum.

OK, I made a patch for ticket #955, and more.  See Original Post. These are my changes:

1. Made a ScuBase.cs.  Moved a lot of the lower level repetitive functionality from StorageScu.cs here.  There is functionality such as timeouts, ability to cancel, etc., that all Scu's that inherit from this class can have for free.

2. Modified StorageScu.cs to use the Asynch / EventHander pattern.  You can either use Send() to send synchronously, or BeginSend() to send asynchronously.  This are breaking chages, it was just too much to keep the old way (if you want, I could put the old way back and deprecate it).  The class comments has an example of verifying asynchronously.

3. Made a VerificationScu.cs class.  The class comments has an example of verifying asynchronously.


4. Made StorageInstance inherit from EventArgs so it could be used in the EventHandler<>.

5. Other various things to make it easy to add files, use the FileList as a property, as discussed above.

Did some testing, will do a lot more too.

Also, wanted to ask, do you have a Coding Standard, as far as variable names, name of order of regions to put properties/methods, etc?

Thanks,

Dan

Hi Dan,

Regarding your request for coding conventions, see this post.

N.

OK, thanks,

I updated the code to follow the standards, or so I hope.

I also made ScuBase and VerifcationScu and StoreScu implement IDisposable... attached is a new patch (I deleted the one from my first message).

Hi Dan,

As mentioned in my response on Oct. 4th in response to your original forum post, I had recently done a number of updates to the StorageScu component (some in response to your original forum post). Your patch is against rev 3474, and my changes were checked in rev 3484. Would it be possible for you to re-submit the patch against the latest source? Unfortuanately, the changes i had done (I had re-arranged some of the method in the source file) make it difficult to reconcile the patch and I thought you could probably do it easier than I can.

Thanks,
Steve

Hey Steve.

I must have missed your changes, I thought I checked with TortoiseSVN and I didn't see anything.  Anyway, I am still getting up to speed with Tortoise so I probably did something wrong or overlooked it.

Anyway, as per your request, a new patch from the latest version.  Hope the changes I made are satisfactory for you.  (Your calling code will change slightly, I don't know if you want this, but see the comments for the class for an example on how to use the new changes)

Dan

Dan,

First, thanks for the contribution, we appreciate it!

I did have a couple of comments on the patch:

First, the use of the Timer in ScuBase probalby is not necessary. There are a number of timeouts in ClientAssociationParameters that contorl timeouts within DicomClient. When a timeout occurs, the OnDimseTimeout() routine is called. Unless you think otherwise, I'll remove the use of the Timer, and instead trigger a timeout condition when OnDimseTimeout() occurs.

I've realized after looking at your changes to the code that the DicomClient code does not have a real good way to clean up after itself. This is really revolving around the CloseDicomClient() routine in ScuBase and the DicomClient.Close() routine. DicomClient.Close() does a direct closing of network connection when its called, no matter what other threads are doing. This routine probably should be protected and not be used by external code. I had originally intended that closing of the connection is really triggered by DicomClient.SendAssociateAbort() or DicomClient.SendReleaseRequest(). In any case, I'm going to look at this closer, and come up with some better cleanup code in DicomClient and DicomServer.

I had recently added a new overload to the ClientAssociationParameters constructor that does not require an IPEndPoint as a parameter for the remote host, instead you can just pass in the hostname and port. This eliminates the need for the GetIPAddress routine you have in ScuBase. I also have put an optimization in this method that it first checks if the hostname passed in an IP address, and if so, it doesn't even go to DNS at all. We were having some problems with that.

The way you refactored the Cancel() routine in broke the functionality in the original code. Ie, the code in StorageScu.OnReceiveResponseMessage() checks to see if the cancel flag is set, and if so fails the remaining storage instances (calling the OnImageStoreCompleted routine), and then cleanly releases the connection. the Cancel() routine also has the issue that the network connection is shut down right away, because of the call to CloseDicomClient().

I'm going to cleanup the above issues, let me know what you think about the timeout.

Steve

Hi Steve,

Great, I figured there was a lot of the lower level things you knew a lot more about that would help make it more solid. 

1.  I wonder if the timeout I put in there is something different than what you are thinking of.  We usually have a timeout so if an operation takes longer than expected (ie, the timeout that we specify), then it times out, instead of hanging forever and forever.  This is helpful when batching sends, if something happens on the network or something, then we don't just let one send hang forever.  If there is something similar already in the code, how can I set a timeout for the client?

2. Yeah I was hoping you had a nice way of cleaning up the DicomClient... I wasn't too sure about the lower level stuff.  Hopefully there is a way to make sure it is cleaned up, whether successful operation, canceled, timed out, etc.

3. Great...

4. OK, thanks for finding that.

Regards,

Dan

Hi Dan,

Concerning the timeouts, for clients there are three timeouts set by properties of the ClientAssociationParameters class, they are the WriteTimeout, ReadTimeout, and ArtimTimeout. The WriteTimout is the timeout used when writing data to the network, the ReadTimeout is the timeout used when reading from the network, and I just realized the ArtimTimeout currently is not being used (so I'm going to remove it). In any case, if any of these timeouts occur, The OnDimseTimeout() method is called to signal the timeout.

If there was some sort of bug iin the DICOM library, its possible that the timeout you've added would catch it, however, the way the library is designed, these properties should prevent the library from hanging forever.

Steve

Hi Dan,

I went ahead and committed the changes. Please take a look and let me know if you have any comments.

Thanks,
Steve

looks good... will let you know of any issues... thanks..

i did see one issue, i forgot to update the comments, some say FileList instead of StorageInstanceList which name had changed...

the only other thing i forgot to mention, Verification and Store is a bit inconsistent, with Store, we put the AETitle, Host, Port, etc. in the constructor, in Verification, we specify those with the Verify() method. Also, could be useful to specify those in Verify() method so we can call it multiple times. Store is a big operation that requires the whole class.

Dan

Hi Dan,

I thought I had updated all the FileList comments, but you're right, I missed a few of those. The commit I did earlier today fixed these comments.

I saw that inconsistency too witht he Verify() method. Technically, the AETitle/Host/Port can be updated between calls to verify through the properties for these values in ScuBase. So, it could be reused by just changing these values. It adds a few lines of code, but it would be possible to reuse the object through those properties. Given that, what do you think about changing the constructor for the VerificationScu to include these values?

Steve

Steve

Hi Steve,

Well, I think it is useful to be able to be able specify the aetitle, hostname, and port on a method instead of having to call 3 separate properties.  I definitely need to be able call verify a few times from our dicom server management page.  Would be nice  to just call a method and pass in the params, but I guess I could live with setting the properties.

I guess we could do both, have the constructor w/ the params, and then have a method with no params, and a method with params?  I dunno, maybe that is overkill and polluting the class.

Dan

Hi Dan,

I think we can just leave it as is, with the Verify() method with the parameters. Verification is a simple enough operation that this is probably fine to leave as is.

Steve

Hi,

I made a UI for the dicom sending, and it would be great if we had an event when an Image Store is started... I was thinking of making the ImageStoreCompleted more general as a ImageStatusChanged, and when a store starts the status could be started.  However, the status is actually a DicomStatus class with the DicomState enum, which doesn't has a Pending state but not an InProgress one... So, I guess it would be best to make a new ImageStoreStarted event?   Or... ?

Also, I forgot to make the StorageScu.OnImageStoreCompleted() virtual, because I want to override that event in a base class.  (I can add this to whatever I changes I make for the started event)

Thanks,

Dan

I think I would prefer to have a new event for this. I would suggest making it something like OnAssociationNegotiated() and put it in BaseScu. The OnImageStoreCompleted() routine has a specific use case (at least for us right now) -- give feedback so that pending statuses can be sent for a C-MOVE request. I'd prefer to have this event stay focused in on that use case, and giving general information when an image store has completed or failed. We can add other events as needed.

Also, no problem on the virtual declaration for StorageScu.OnImageStoreCompleted().

Steve

Steve,

What kind of params do you want for an OnAssociationNegotiated() in BaseScu?  I don't see how it will help me get an event for when each image is started being sent.

Thanks,

Dan

I'm sorry, I missed your point. When I wrote my response, I was thinking that you wanted to have an event before the first image was sent, and not before every image being sent. Your original suggestion makes sense.

Steve

OK, see attached patch , with the new ImageStoreStarted event and changing OnImgeStoreCompleted to virtual.

btw, I was thinking of cleaning up the code for maintenance sakes, there is a bit of code that is repeated 3 times, but I wasn't sure exactly what they were all doing and I didn't want to break anything:

            bool ok = false;
            while (ok == false)
            {
                _fileListIndex++;
                if (_fileListIndex >= _storageInstanceList.Count)
                {
                    Platform.Log(LogLevel.Info, "Completed sending C-STORE-RQ messages from {0} to {1}, releasing association.",
                        association.CallingAE, association.CalledAE);
                    client.SendReleaseRequest();
                    StopRunningOperation();
                    return;
                }

                ok = SendCStore(client, association);
            }


Maybe you can take a look and cleanup if possible?

Regards,

Dan

Hi Dan,

Thanks again for the contribution, it's apreciated!

I went ahead and applied your patch.  As of a few minutes ago its been committed.  A couple of comments:

- I added a call to OnImageStoreStarted in the FailRemaining() method.  OnImageStoreCompleted is called when we fail each of the remaining storage instances when a failure occurs on the association, so I thought it made sense to call OnImageStorageStarted also.

- As you suggested, I refacted the loop code that was in OnReceiveAssociateAccept and OnReceiveResponseMessage.  This code is now in the function SendCStoreUntilSuccess.  The purpose of this loop code was to go to the next file in the StorageInstanceList if we had some sort of failure sending the current instance.  This may be because the sop class of the file we're trying to send wasn't negotiated over the association, or if there was some sort of error reading the file off of disk.

In any case, I'm going to put the ticket in test.  If we make any more changes to the StorageScu, I'll create a new ticket.

regards,
Steve