jakartaee / jsonp-api

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

Fix JsonGenerator.close regression #404

Open rmannibucau opened 10 months ago

rmannibucau commented 10 months ago

Please check https://github.com/jakartaee/jsonp-api/issues/156, this issue introduced by a too quick review a big regression on JSON-P API design (and implementations/doc bugs) so it is more than likely and welcomed to revert this by documenting properly the behavior and harnessing it instead of breaking the JSON-P api design.

jeanouii commented 10 months ago

Actually I don't think it should close the underlying Writer/OutputStream at all. The JsonGenerator isn't the owner of the stream and should never close it, on exception or not. It's easy to leak resources when the code is asymmetric and it's also very hard to track/debug.

My golden rules

Don't delegate the closing of your resources to someone else. Don't close someone's else resources.

The caller passes in the OutputStream or the Writer when creating the JsonGenerator, so the caller is responsible for closing it properly (one can use try-with-resource).

The #close() on JsonGenerator must only close the resources it created (buffer and others).

jeanouii commented 10 months ago

In other words, there is something to fix, but I would not revert the change, I would remove the close on the underlying stream at all. And make it clear in the javadoc that it's under the caller responsability to monitor and enforce the lifecycle of the resources it opens.

rmannibucau commented 10 months ago

@jeanouii does not look consistent with the api, JsonGenerator is designed to be autocloseable and to wrap an underlying stream, if it does not close the stream it does leak, even tck are written this way (https://github.com/jakartaee/jsonp-api/blob/4d578207c6443056d5e3ddc22d3acba517181a7b/tck/tck-tests/src/main/java/ee/jakarta/tck/jsonp/api/jsongeneratortests/Generator.java#L97). Indeed it can be rediscussed in a v3 but not in a minor IMHO.