Don't catch OutOfMemoryError

[Marcono1234]

Gson currently catches OutOfMemoryErrors at two places and wraps them inside JsonParseException:

gson/gson/src/main/java/com/google/gson/JsonParser.java

Lines 88 to 89 in c5a3f21

	} catch (OutOfMemoryError e) {
	throw new JsonParseException("Failed parsing JSON source: " + reader + " to Json", e);

gson/gson/src/main/java/com/google/gson/JsonStreamParser.java

Lines 90 to 91 in b75e1bb

	} catch (OutOfMemoryError e) {
	throw new JsonParseException("Failed parsing JSON source to Json", e);

This is bad practice because Gson is not necessarily the cause for this error and wrapping it inside an Exception subclass prevents the caller from noticing the OutOfMemoryError until later.

Edit: Catching OutOfMemoryError might be desired to protect about malicious JSON data, see also #1912 (comment). However, it appears OutOfMemoryError is not caught consistently everywhere.

[lyubomyr-shaydariv]

I think those were implemented with a good intention in mind:

• The parsers return JSON trees that can be big enough not to fit in the memory. Say, you have a method static long[] allocate(final int length) { return new long[length]; } and pass a big enough length argument, so that JVM won't be able to allocate the array. This does not mean that your program is going to die: the array won't be allocated and the heap would remain intact, and the JSON trees, generated by those parsers, would be collected by GC in case of OOM anyway (I assume so, to be honest).
• This change might change how people catch exceptions making them catch errors or Throwable directly that is a bad practice too. I'd rather catch a domain-specific JsonParseException (despite it's a RuntimeException subclass) right at invoking the parseReader method than bothering for OOM that might be a result of too big JSON trees catching OOM elsewhere.
• StackOverflowError is wrapped in a domain exception too, though.

Thoughts?

[Marcono1234]

For StackOverflowError it is similarly questionable because it could make noticing infinite recursive more difficult. However, there it is at least in some way related to Gson (even if Gson is called at the end of a very long call chain). It has no effect on other threads or the application as a whole.

However, for OutOfMemoryError, Gson (or a type adapter or similar) might be responsible for it, but there is no guarantee. If for example 99% of the available memory is already used, and then Gson tries to allocate memory and fails, it is not Gson's fault. Catching the error here prevents the caller from noticing this problem and handling it properly, e.g. shutting down the application, logging a fatal error message or similar. Maybe the caller catches JsonParseExceptions and just logs them as warning, after all, JsonParseExceptions are also thrown for something as mundane is invalid JSON. They would never notice the big problem they are having.

If every library was wrapping OutOfMemoryErrors, applications would constantly log warnings, but would never notice that the complete application is not functional anymore.

[wind57]

the argument against catching an OutOfMemory in any library code is strong. The only place I would be aware of where such a thing would potentially make sense is the very top level "catcher" in an application, for logging purposes only.

[lyubomyr-shaydariv]

@wind57
I disagree as it depends: the OOM caught in the code is an attempt to detect whether JSON tree to read can fit in the heap. Unfortunately Gson is not really able to detect whether it caused the OOM itself or the OOM is caused by non-Gson stuff, and this is really unreliable. If your code is attempting to allocate a huge array, string, collection, or a huge object of any other kind in very nested code, it does not mean that your application is running out of memory entirely (i.e., there are dozens questions at StackOverflow where people are attempting to store incoming JSON documents into huge strings -- Gson does not fail, but the StringBuilder does), and your nested code is likely aware of the intention and the code that can potentially need to allocate huge objects just because it is asked to, and if such a component can deal with it, it may decide whether not top-level logging is appropriate or the component can resolve the error itself. It really depends.

[wind57]

Unfortunately Gson is not really able to detect whether it caused the OOM itself or the OOM is caused by non-Gson stuff you said it yourself. I hear your reasons, I understand them; but I do not agree with them. Will see where this goes... thx.

[lyubomyr-shaydariv]

@wind57
just to be clear: I didn't design it, but I'm trying to reconstruct the rationale behind the design choice and trying to figure out why several errors (OOM is just one of them) are caught right there, despite most sources recommend never catch these but log them at the very top level (or just let the JVM to crash). Perhaps catching the OOM error might be fine when trying to allocate too lengthy memory blocks (like arrays I was thinking of above or any other objects that may require such arrays under the hood (huge bitmaps/images, huge strings, etc: https://stackoverflow.com/questions/2679330/catching-java-lang-outofmemoryerror)) and not work fine when trying to catch an OOM error while constructing a complex/fragmentary object built of too many small blocks like JsonElement hoping that GC would collect such a JSON tree once it fails to construct, so the process would survive. But yes, we'll see.

[Marcono1234]

Catching StackOverflowError also has a security aspect, but I am not sure if that was the intention (it is definitely not made explicit): When Gson is used to parse untrusted JSON a malicious user could send deeply nested objects or arrays which might trigger a StackOverflowError. Since the application likely is not handling java.lang.Error this might cause the complete application to crash.