Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JsonGenerator.close regression #404

Open
rmannibucau opened this issue Oct 10, 2023 · 3 comments
Open

Fix JsonGenerator.close regression #404

rmannibucau opened this issue Oct 10, 2023 · 3 comments

Comments

@rmannibucau
Copy link

Please check #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
Copy link

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
Copy link

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
Copy link
Author

@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 (

try (JsonGenerator generator = Json.createGenerator(strWriter)) {
).
Indeed it can be rediscussed in a v3 but not in a minor IMHO.

jeanouii added a commit to jeanouii/johnzon that referenced this issue Oct 12, 2023
…lose the underlying stream

Signed-off-by: Jean-Louis Monteiro <jlmonteiro@tomitribe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants