Throw MJsonException only.

[unserializable]

^^ Reasoning for single exception: it makes sense to have one specific exception to catch when invoking tiny specialized library. ^^

Exception handling changes:

• added MJsonException
• throw MJsonException (also unchecked exception) instead of RuntimeException, when mJson encounters error condition it cannot handle and that is not illegal argument / unsupported operation exception.
• replaced dangerously empty catch (Throwable t) {} clause with normal handling of IOException
• throw MJsonException instead of IllegalArgumentException
• throw MJsonException instead of UnsupportedOperationException

Additionally:

• addressed build warnings about pom.xml deprecated ${groupId}+ ${artifactId} references which were replaced with current ${project.etc} syntax (Maven 3.2.2 warning)

[unserializable]

Any plans to merge these changes?

[bolerio]

I look into it shortly. I apologize that I seemed to ignored, but for some reason this has passed under the radar, thanks for reminding me!

[unserializable]

Ok. Note that (it comes to me as bit of a surprise) that this pull request from April now also involves my additional commits from day before yesterday..., since all work was done on master branch.

[bolerio]

I commented on specific commits I have issues with. The other commits I haven't commented on are good and I will merge them.

[unserializable]

Hey Borislav, on the finally block exception catch principle I am fifty-fifty, it is a matter of principle of whether to put up with flaky Readers :) However, it should not be 'catch (Throwable t), as the catch is that it catches all kinds of horrenduous OutOfMemoryErrors, VirtualMachineErrors etc.. silently and can cause days of clueless debugging when part of larger system failure.

On the reasoning -- catching these exceptions usually happens when playing with code or writing tests and then having to juggle around IllegalArgumentException/UnsupportedOperationException. But generally -- if there is a really tiny library, I'd rather have ONE exception coming from it, with a meaningful message.

Ditto on the Json.Factory :)

[bolerio]

catch (Throwable) will catch both Error instances and Exception instances thus ensuring that a finally block is doing a cleanup and any errors or exceptions that were caused by the code doing actual work do not get overridden. This is very important. The scenario where the try {} block is fine, but what would have been a normal reader.close actually results in a JVM error is unlikely. The opposite, where an actual problem in the try {} block becomes overridden seem more likely to me - because I've seen it happen, on several different occasions. But I'm willing to change my mind if I see a case showing a problem with a catch (Throwable t) during a close.

I guess what could be done is t.printStackTrace(System.err) so there's a record somewhere in a properly deployed application. But hiding the root cause potentially coming from the try block would be a idea in my opinion.

[unserializable]

"catch (Throwable) will catch both Error instances ... ensuring that a finally block is doing a cleanup"

^^ Yeah, thats why Throwable should not be caught silently -- there is no guarantee that application can even continue to run after Error and any cleanup routines being able to execute is doubtful. When Error gets silently swallowed, the end result can easily be a hundreds of megabytes of Java code sitting in the process, doing nothing -- and nobody knows why (true story).

On the t.printStackTrace -- that is not guaranteed to execute when Error occurs either, a lot of new objects are constructed for printing stacktrace so e.g on OutOfMemoryError it would hopefully throw another Error from the handler but it might not have resources for that ;) Even if that Error is let to propagate, then there seems to be just /chance/ that application container / VM handler is able to actually log/print the Error. Chance is better than no chance though.

On Error handling: https://docs.oracle.com/javase/8/docs/api/java/lang/Error.html http://stackoverflow.com/questions/11017304/catching-java-errors#11018879

[bolerio]

"When Error gets silently swallowed, the end result can easily be a hundreds of megabytes of Java code sitting in the process, doing nothing -- and nobody knows why (true story)" : that's interesting, can you tell me what the actual error was? I can think of other scenarios where it would make sense not to swallow even an exception from reader.close() - a buggy reader implementation that fails to close even though there was no exception in the try {} block, so resources get leaked and you never know why or where. So, yes, there are scenarios like this. But a close operations is generally supposed to be freeing resources and the likelihood of it being the root cause of a problem is very, very small.

The main issue here is that we are in a finally block. The general rule of "never swallow exceptions let alone JVM Errors" is not so firm in this case because as I mentioned we'd be hiding the root cause of a problem. Also, unfortunately Error instances don't always indicate a JVM error because many library implementors wrongfully choose to extend Error. Just from the few projects I have currently in Eclipse, if I look at the hierarchy, I can see a bunch, HttpClientError from apache commons-httpclient being one for example. Not sure if you understand correctly my point, but I'm not saying it's ok the swallow a throwable because it'll not happen or it's not important or whatever. On the contrary,I'm trying to prevent the more common and important throwable, the one coming from the try block, from being in effect swallowed, which is what'll happen if we throw something else from the finally block.

Cheers

[unserializable]

Hey Borislav,

I was not doing the headnumbing debugging for the case mentioned, but it was one of the VirtualMachineErrors.

It seems to me this comes down to whether one considers Errors actually serious things or believes that concept is so often used in flawed way IRL that they should not be taken seriously. My take is that Errors are meant to be serious and should be treated as such, even when some library happens to use them incorrectly.

Then swallowing some Exception from try{} is not a good reason to possibly mask an Error from finally {} with catching Throwable instead of e.g. IOException.

As for commons-httpclient, luckily I have not had to use it and now it has been superseded by Apache HttpComponents :) That mention of HttpClientError sounded really bad, but when I had a look where commons-httpclient 3.1 actually uses the HttpClientError, it is reasonable -- in 2 cases thrown when constraints required by Java platform are not satisfied in runtime and 3rd case de facto same:

• MD-5 MessageDigest cannot be created (MD-5, SHA-1, SHA-256 are among the ones that Java platform is required to support) in org.apache.commons.httpclient.auth.DigestScheme

final String digAlg = "MD5";
MessageDigest md5Helper;

try {
    md5Helper = MessageDigest.getInstance(digAlg);
} catch (NoSuchAlgorithmException e) {
    throw new HttpClientError(
        "Unsupported algorithm in HTTP Digest authentication: "
            + digAlg);
}

• Instantiation of required ISO-8859-1 charset encoding fails in apache.commons.httpclient.util.EncodingUtil

try {
    return doFormUrlEncode(pairs, DEFAULT_CHARSET);
} catch (UnsupportedEncodingException fatal) {
    // Should never happen. ISO-8859-1 must be supported on all JVMs
        throw new HttpClientError("Encoding not supported: " + 
             DEFAULT_CHARSET);
}

• Instantiation of SSLContext "SSL" fails in org.apache.commons.httpclient.contrib.ssl.EasySSLProtocolSocketFactory -- this one can be considered slightly fishy, since "TLSv1" is only one required but "SSL" is specified among the names that can be used to instantiate SSLContext.

try {
    SSLContext context = SSLContext.getInstance("SSL");
        context.init(
            null, 
            new TrustManager[] {new EasyX509TrustManager(null)}, 
            null);
    return context;
} catch (Exception e) {
    LOG.error(e.getMessage(), e);
    throw new HttpClientError(e.toString());
}