Jsonb close contract should be revisited

[marschall]

The Json methods that take IO streams should have their #close() contract revisited.

The current situation is:

• The methods taking an InputStream or OutputStream as an argument specify "Upon a successful completion, the stream will be closed by this method."
• No contract exits for the method taking a Reader or Writer as an argument.

The issues with this are:

1. If the method throws an exception it is not specified wether the streams are closed. This leaves it unclear whether the caller is responsible for calling #close() in these cases.
2. It is inconsistent that the byte oriented streams (InputStream / OutputStream) are closed but the character oriented streams (Reader / Writer) are not.

Secondly, it is debatable whether Json should call #close() on the streams passed at all or whether that should be left to the caller. In my personal view it is idiomatic Java if the creator of a "resource", eg. IO stream, is responsible for its closing. Meaning generally calling code should look like this

try (var ioStream = createIoStream()) {
  jsonb.toJson(object, ioStream);
}

This also integrates well with static analysis tools looking for resource leaks. If a user wants to keep the IO stream open, eg. to implement line oriented JSON, they can do this by simply not calling #close(). Otherwise they would have to wrap the IO stream with one suppressing #close().

This is similar to C where in general code calling malloc is also responsible for calling free. Functions are in general not expected to clean up memory they are passed.

I could find no written rule or recommendation for this but the majority of the JDK code I can find does not close IO streams passed as an argument and leaves calling #close() to the caller.

Examples from the JDK where calling #close() is left to the caller:

• Properties#load(Reader)
• KeyStore#load(InputStream, char[])
• javax.imageio.ImageIO#read(InputStream)
• java.util.logging.LogManager#readConfiguration(InputStream)
• javax.tools.Tool#run(InputStream, OutputStream, OutputStream, String...)

Examples from the JDK where #close() is only called if the method returns without throwing an exception (strict interpretation of the current Jsonb API contract)

• Properties#loadFromXML(InputStream)

Examples from the JDK with no specified contract for calling #close(), likely left to the caller:

• javax.script.ScriptEngine#eval(Reader)
• javax.xml.parsers.DocumentBuilder#parse(InputStream)

See also https://github.com/eclipse-ee4j/yasson/pull/586

[rmannibucau]

Hi

Guess reader/writer should be aligned on input/outputstreams to stay backward compatible, not sure there is a value to break existing code again even if your reasoning vs current behavior can be debatable (50-50 regarding existing api so hard to favor one IMHO so I prefer to not break).

[jamezp]

I'd argue the other way around and that InputStream and OutputStream be aligned with the reader/writer. Yasson, the RI, didn't close these streams until 3.0.3. Either way you're technically breaking compatibility so it's better to do it right.

[rmannibucau]

@jamezp well, you argue "in theory", in practise it was spec-ed for half of the method so this part will not change, then what can be discussed is the unspec-ed part but I'm pretty sure it is more natural to think streams and read/writ-ers are aligned than assuming the API is inconsistent so back to my previous point until we want to shout yet another time in our feet and break again the API.

[jamezp]

IMO it was only partially spec-ed for half. What I mean is the implementation is only supposed to close the stream "Upon a successful completion" with no indication what to do if the serialization/deserialization was unsuccessful. The user is left to guess whether or not their stream they passed in was closed.

A change to require closing too, requires that Jakarta REST implementations wrap the OutputStream because a MessageBodyWriter explicitly states that the OutputStream should not be closed.

[rmannibucau]

Agree the success only is broken and does not work but still user assumption is clearly he must not own the close by spec so our only choice for compat is to always close.

Agree on jaxrs but same there, this got solved in jaxrs layer in 1.0 by using wrappers flusing on close so no issue.

[jamezp]

Agree to disagree I guess :) 2 out of the 8 scenarios are covered by the JavaDoc.

Hopefully others will give an opinion here as well and we can at least make things consistent.

[rmannibucau]

@jamezp don't forget to mention that the 6 cases are unspecified which means we breaks no more going one way or the others whereas we break if we specify the other way around the 2 covered methods and that taking into account fair assumption (consistency of the API) we also break the 6 unspecified cases.

Side note: I never said I'm happy with that state but I would be very unhappy to break again end users code for something so insignifiant in an app.

[bmarwell]

As a user, I always use try-w/-resources on everything I pass into any library function. If some consumer closes my streams, I'd be surprised.

Imagine I need to read it twice and wrap it into a ReadBackBAStream. If the consumer closed that one, I'd be very unhappy.

I don't really care about the backwards compatibility in this very case. I care about "doing things the right way". Don't do something which is really uncommon, please.

[marschall]

As a user, I always use try-w/-resources on everything I pass into any library function. If some consumer closes my streams, I'd be surprised.

I feel the same way. However I struggle to find an authoritative reference (well known book, talk, presentation, person, developer, ...) that documents this behavior. If you have something please feel free to share it here.

Edit:

This also is the behavior the TCK is doing.

https://github.com/jakartaee/jsonb-api/blob/7340b84ec2a5bbdeb492d425eccee6d65443e97c/tck/src/main/java/ee/jakarta/tck/json/bind/api/jsonb/JsonbTest.java#L133

[marschall]

Here are TCK tests for the behavior as currently speced according to my reading #351.

[KyleAure]

Sorry I haven't kept up much with this discussion. I agree with the statement:

it is idiomatic Java if the creator of a "resource", eg. IO stream, is responsible for its closing.

The Jsonb methods should not be closing resources, the methods who create the resources should. I also agree that Jsonb should standardize this behavior for all resources.
It doesn't make sense (to me), no matter which decision is made, to treat streams and readers/writers differently.

This would also help clear up the need for me to handle this situation:

Upon a successful completion, the stream will be closed by this method.

Today I'd have to write something like this:

OutputStream out = createIoStream();
try {
  jsonb.toJson(object, out);
} catch (jsonbException) {
	//handle exception
	out.close();
}

Whereas I'd like to write:

try (OutputStream out = createIoStream()) {
  jsonb.toJson(object, out);
} catch (jsonbException) {
	//handle exception
}

The second is easier to write, easier to understand, and makes it so I cannot forget to close my stream.

[marschall]

Some further arguments against closing

The toJson methods of YassonJsonb leave the parsers and generators open. For the generators this is mentioned in the API contract.

The generator is not closed on a completion for further interaction.

The json parsers are also not closed, but this is not specified in the API contract.

Some further arguments for closing

JsonParser and JsonGenerator have #close() methods that are specified to close the underlying stream with no way to suppress this other than wrapping the streams.