jakartaee / jsonp-api

Jakarta JSON Processing
https://eclipse.org/ee4j/jsonp
Other
141 stars 61 forks source link

What is the contract for of JsonGenerator#close() on error? #156

Closed jjspiegel closed 3 years ago

jjspiegel commented 5 years ago

JsonGenerator.close() is defined to close the underlying output source. https://javaee.github.io/javaee-spec/javadocs/javax/json/stream/JsonGenerator.html#close--

And it is also defined to raise an exception if an incomplete value is generated: JsonGenerationException - if an incomplete JSON is generated

So, if while writing to a generator, an exception is raised (either from a generation method or from some other cause) calling close() on the partially written generator is very likely to raise another exception.

The question is what is the contract for the underlying output source in this case? Consider the following examples.

In the reference implementation, the following example will not close System.out:

  try (JsonGenerator gen = Json.createGenerator(System.out)) {
    gen.writeStartObject();
    gen.write("foo", "bar");
    // no end object
  } catch (JsonGenerationException e) {
    System.out.println("Gen error: " + e.getMessage());
  } 

However, the following example will close System.out:

  try (JsonGenerator gen = Json.createGenerator(System.out)) {
    gen.writeStartObject();
    gen.write("foo", "bar");
    gen.writeEnd();
  } catch (JsonGenerationException e) {
    System.out.println("Gen error: " + e.getMessage()); // no error in this example
  } 

Is this the intended behavior?

More realistically, a generator may be receiving events from an external source when an error occurs. Given this behavior, it seems useless and potentially troublesome (due to the assured double-error) to close the generator in this case.

m0mus commented 5 years ago

I guess Javadoc is not clear enough. The underlying stream is closed only if complete JSON object is written. In case of incomplete object JsonGenerationException is thrown and underlying stream is not closed.

jbescos commented 3 years ago
lukasj commented 3 years ago

done

rmannibucau commented 1 year ago

Hi,

seems this introduces a lot of bugs cause JsonGenerator design is to close in all cases the underlying stream (otherwise you can never use the generator as an autocloseable which is the opposite of what we wanted/want). It is also a very big regression from previous versions and creates leaks.

Therefore this must be fiexd in 2.1.next asap to enforce to always close the underlying stream by spec as before.

Note: it is more than fine to swallow the close exception is any and use addSuppressed if it helps.