Open Marcono1234 opened 4 years ago
I think those were implemented with a good intention in mind:
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).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?
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 JsonParseException
s and just logs them as warning, after all, JsonParseException
s 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 OutOfMemoryError
s, applications would constantly log warnings, but would never notice that the complete application is not functional anymore.
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.
@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.
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.
@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.
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.
Gson currently catches
OutOfMemoryError
s at two places and wraps them insideJsonParseException
: https://github.com/google/gson/blob/c5a3f21fbad3828deba1deb88168e1f76829f807/gson/src/main/java/com/google/gson/JsonParser.java#L88-L89 https://github.com/google/gson/blob/b75e1bbc7915f409ba0254fcc92c71ed6798e69b/gson/src/main/java/com/google/gson/JsonStreamParser.java#L90-L91This 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 theOutOfMemoryError
until later.Edit: Catching
OutOfMemoryError
might be desired to protect about malicious JSON data, see also https://github.com/google/gson/pull/1912#issuecomment-1130767871. However, it appearsOutOfMemoryError
is not caught consistently everywhere.