Prevent non-socket related exceptions from being swallowed

[RyanMelenaNoesis]

Allow useful exception messages to be passed out of fo-dicom to calling application.

[anders9ustafsson]

Many thanks for offering this PR. We will review it in detail as soon as possible.

Best regards,
Anders @ Cureos

Sent from my iPhone

[anders9ustafsson]

@RyanMelenaNoesis I disapprove of removing the readonly declaration, since it would break encapsulation. What is the real issue here? What kind of non-ASCII encoding are you encountering with the MWL records? Why do you have to change the Default encoding to be able to deal with these records?

[RyanMelenaNoesis]

The situation we've actually encountered is Modality Worklist Servers which return ISO-2022-JP encoded DICOM documents but lack the Specific Character Set tag. In these cases, despite the Specific Character Set tag being empty, the encoding is known ahead of time and can be configured in our application. As far as I can tell, without the ability to set the Default encoding we'd be forced to manually convert every value we read from the DICOM document to the correct encoding. Furthermore, if the underlying encoding were non-ASCII-based (ISO-2022-JP happens to be ASCII-based) and the Specific Character Set tag was empty there would be no way to correctly retrieve information from the DICOM document as the data would already have been converted to ASCII and information would be lost.

I would disagree with the assertion that this change breaks encapsulation as the Default property is already public and is used by other classes in the library. I believe it is similar to the DicomImplementation.ClassUID property. However, we would certainly be open to suggestions as to how to handle the situation described above.

[anders9ustafsson]

@RyanMelenaNoesis Sorry if my wording was unclear. What I mean is that I disapprove of making the DicomEncoding.Default field mutable. Generally I consider it bad design to allow for modification of globally accessible fields, and I honestly think that it is an oversight that the DicomImplementation.ClassUID and DicomImplementation.Version fields are mutable in the current implementation.

I (think I) understand your problem, but my personal opinion is that we should try to find a more trustworthy solution to this issue, rather than making a "quick hack" that might potentially have big implications of the overall reliability of the library.

Is this issue in any way related to the already reported issue #43 (originally posted in Google Groups)? If not, I would prefer that the default encoding commit is removed from this PR and that the problem is instead logged as a separate issue that we should strive to resolve as expediently and reliably as possible.

Once again, this comment reflects my personal opinion. If someone else in the community disagrees with my conclusion, do not hesitate to post a comment, I am fairly open to reason :-) . In particular, @hdesouky @IanYates @GMZ .

[RyanMelenaNoesis]

I would agree that the configuration via static class properties isn't ideal. It would be nice to have a centralized configuration interface. Even better would be to have a central configuration interface that took providers. Something like:

public interface IFoDicomConfiguration
{
    Encoding DefaultEncoding Func<Encoding> { get; }
}

with a default implementation like:

public class DefaultFoDicomConfiguration : IFoDicomConfiguration
{
    Encoding DefaultEncoding Func<Encoding>
    {
        get { return () => Encoding.ASCII; }
    }
}

That, however, seems like a significant change and it would be nice to have a way to address the situation outlined in the meantime.

I do not believe this is related to the issue #43.

[anders9ustafsson]

@RyanMelenaNoesis In principle I approve of the OnConnectionClosed signature change. However, some concerns:

1. Are there situations where an errorCode is meaningful, whereas an exception is not, or does such a scenario not exist?
2. The signature change breaks the API. @hdesouky @IanYates @GMZ is it still OK to accept the signature change in the upcoming 1.1.0, or should we hold the PR back for a later release?
3. It would be good if you could also add a few unit tests to verify the validity of the API changes.
4. The omitted readonly declaration in DicomEncoding.Default: I strongly disapprove of this construct, see my line comment. Unless I can be convinced otherwise, I want to exclude that commit from this PR. @hdesouky @IanYates @GMZ what are your opinions on the matter?

[RyanMelenaNoesis]

In response to your concerns:

1. The errorCode only really made sense in the case of a SocketException as the SocketException class has an ErrorCode property (which other exception types do not).
2. Obviously not for me to decide
3. Could you provide more information regarding what type of tests you're looking for? The change pretty much just changes the error information exposed by the library to the consumer and doesn't seem to be affect internal library behavior.
4. As mentioned above, we're open to suggestions on how best to solve the issue described.

[RyanMelenaNoesis]

Addressed Error.Log syntax issue as well as some extraneous int errorCode variables.

[anders9ustafsson]

@RyanMelenaNoesis Regarding unit tests, No, I don't have any good examples, it is mainly that I want to increase the unit test code coverage overall since it is very sparse right now. If you don't have any relevant unit tests related to the modified code, that is OK too.

[IanYates]

For the overall thurst of this PR, I personally would rather have exceptions thrown rather than error codes - at least with an exception I can do some good logging outside of the library and hopefully see something extra in its properties that can help me fight a fire in production. Null for an exception is probably a better sentinel nul value than 0 for an error code too.

As for having a breaking change to the method signatures, I don't mind. If we have to take a breaking change then we might as well do it before the NuGet package loses its "pre" status. Otherwise a change like this would end up being a v2 thing which could be a fair way down the line.

The default encoding bit is trickier. Both make good points of course :) The global state definitely shouldn't be able to be mutated. Many libraries have an "init" routine that needs to be called but fo-DICOM hasn't usually needed one unless you were configuring logging. We could get such a thing in without changing the current public API by allowing for an init routine and having every publicly available API call some "checkDefaultInit()" method which will perform a default initialisation of the library if one hasn't already been done. That clutters our code with an extra line at the start of the public methods but does allow for a custom init to occur whilst still letting most people accept the defaults.

I've seen some OSS libraries use IL weaving (something like Fody) to avoid that clutter and automatically introduce the one-liner into public methods of attributed classes but that might be a bridge too far :)

[GMZ]

While I like the idea of exceptions, this is not a good option, I have resisted merging the same request to my existing fork of fo. error codes are make a lot more sence when dealing with network code. I feel that this is more of an issue of improper use or to hack around issues in modality vendors software

As for the other I have major issue with it and can't be convinced otherwise, @anders9ustafsson I agree with that this breaks encapsulation, it certainly varies from the standard see the following stackoverflow question on the same topic http://stackoverflow.com/questions/27507090/default-dicom-encoding-without-specific-character-set
This smell of a configuration problem with MWL server... some MWL servers will not echo this tag unless it's supplied in the Query, which may be the case here as an empty tag or no tag would indicate the default encoding I'm apposed to both bits

If nothing else we must ad-hear to the standard or why bother maintaining another broken implementation

[anders9ustafsson]

@GMZ As I see it (and I think @RyanMelenaNoesis agrees) the advantage with passing an exception here is that you get "the best of both worlds"; if it is a SocketException the error code is included in the exception object, and if for some reason it is not a SocketException you can still get relevant information about what went wrong from the exception contents.

Do you see any scenario where passing an exception instead of an error code could be harmful or not applicable?

[IanYates]

I don't feel strongly enough either way but, from my limited real-world use of DICOM compared to others on here I could not see any particular downsides in having exceptions reported rather than error codes.

As for the configuration improvements, again I am not super aware of the spec in this area... I appreciate the other system that resulted in the need for some of this PR is not compliant but, if this problem is common-ish in the wild (is it?) would it at least be helpful to give users of the library a way to make fo-DICOM more liberal in what it accepts?

[anders9ustafsson]

@RyanMelenaNoesis If I understand correctly, you are currently not experiencing any issue with the default encoding?

Furthermore, if the underlying encoding were non-ASCII-based (ISO-2022-JP happens to be ASCII-based)

The readonly removal is only introduced in the eventuality that some MWL server uses non-ASCII based encoding?

Either way, it would be helpful if you could show some of your code where non-ASCII encoding is potentially an issue.

[anders9ustafsson]

If I am not mistaken, the whole encoding issue is due to the fact that the DicomDatasetReaderObserver class uses encoding DicomEncoding.Default unless a specific character set is specified, right?

Instead of removing the readonly declaration on DicomEncoding.Default, I would prefer it if we made it possible to configure DicomDatasetReaderObserver to use an alternative encoding by default, specified for example as an optional argument in the constructor. This calls for a few more code changes, but I consider this a considerably less intrusive approach than allowing DicomEncoding.Default to be re-configured on a global scale.

Feedback, please :-)

[anders9ustafsson]

@RyanMelenaNoesis @IanYates @GMZ I am prepared to cherry-pick the two commits related to the OnConnectionClosed signature, but skip the third commit with the readonly removal. Instead I would like to open a new issue on configuring the default encoding of the DicomDatasetReaderObserver via the constructor (as discussed in the previous comment).

If someone objects to this plan, please let me know.

[anders9ustafsson]

I have now cherry-picked the first and third commit. Will post a separate issue for the encoding business. Thanks, @RyanMelenaNoesis for your contribution!

[RyanMelenaNoesis]

I'd have to look at the code more carefully but so long as an optional parameter to DicomDatasetReaderObserver would allow the specification of an encoding for read operations, that would solve the unicode issue from our perspective.

As for the error code vs. exception changes, thanks for pulling them. In response to @GMZ, we did actually run into an issue where the code in question swallowed a useful, non-socket related, exception which was the impetus for the change.

[RyanMelenaNoesis]

@anders9ustafsson It is true that we have not yet encountered a MWL that is returning non-ASCII encoded documents without the Specific Character Set tag. We have, however, encountered real-world MWL servers that return ASCII-based (ISO-2022-JP) documents without the Specific Character Set tag.

In the latter case we can still successfully display the dicom data, given foreknowledge of the implicit encoding, because the conversion (by fo-dicom) of the original ASCII-based (ie ISO-2022-JP) data to ASCII does not involve any loss of data.

It seemed likely to us that if an MWL serving ASCII-based documents behaves in such a way there might well be an MWL serving non-ASCII documents which behaves that way as well. If such an MWL were encountered we would be unable to display the dicom data, even with foreknowledge of the implicit encoding, because the conversion (by fo-dicom) of the original non-ASCII data to ASCII would involve data loss. In the case of .NET that data loss, by default, is a conversion of all byte values outside the normal ASCII range to a '?'.

We don't disagree with @GMZ's assessment that this behavior seems to put the MWL out of compliance with the DICOM standard but it sure would be nice to have some way of overriding the fo-dicom behavior, no matter how cryptic, to avoid data loss were such a non-compliant MWL encountered. An added bonus would be that the same mechanism should prove handy for the real-world ASCII-based (ISO-2022-JP) cases we've witnessed as it would prevent the need to do a manual encoding transformation on each field read from the non-compliant DICOM document.