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

Close Parsers and Generators #586

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Conversation

marschall
Copy link
Contributor

Close parsers and generators to return buffers to the pool.

JsonBinding currently does not #close the parsers and generators is creates resulting in org.eclipse.parsson.api.BufferPool#recycle(char[]) not being called resulting in unnecessarily high allocation rates

Close parsers and generators to return buffers to the pool.
@jamezp
Copy link

jamezp commented Oct 6, 2023

Was there an issue filed for this? It's closing streams it may not own causing issues in containers. The linked SpringBoot issue is an example. I'm seeing the same thing in WildFly/RESTEasy with some tests.

@jamezp
Copy link

jamezp commented Oct 6, 2023

Just to follow up, the WildFly/RESTeasy regression will be fixed by RESTEASY-3397. This fix is correct according the Jsonb.fromJson() JavaDoc.

@marschall
Copy link
Contributor Author

Was there an issue filed for this?

There was not. Sorry for the trouble, I did not consider the possible impact this may cause to consumers downstream.

It's closing streams it may not own causing issues in containers.

Reading the Javadoc of jakarta.json.bind.Jsonb in more detail several issues strike me:

  1. Only the InputStream and OutputStream based methods have in their contract that the parameters will be closed. The Reader and Writer based methods do not. Which leaves the API user to guess with is never a good sign.
  2. The Javadoc specifies "Upon a successful completion" which makes it unclear whether the stream will also be closed in case the method throws an excepiton.
  3. In my view idiomatic Java is when the creator of a resource (stream, ...) is responsible for its release, see Properties#load(Reader) or KeyStore#load(InputStream, char[]). I don't have a copy of Effective Java at hand right now.

Should we raise these with https://github.com/jakartaee/jsonb-api?

@jamezp
Copy link

jamezp commented Oct 9, 2023

@marschall That's a good catch. I was in a rush on Friday when I was attempting to figure out what the issue was. It does make sense to make the Reader and Writer versions consistent.

Maybe it is a good idea to keep them open like Properties.load() does as that is a similar type of method. Filing an issue does make sense to me.

@jamezp
Copy link

jamezp commented Nov 14, 2023

Will this change by chance be reverted or partially reverted to not close the streams?

@marschall
Copy link
Contributor Author

If there is tentative agreement that the streams should remain open and the spec and TCK should be updated accordingly I can work on a PR that restores the original behaviour regarding streams but closes the parsers but still returns the buffers to the pool.

@dasteg
Copy link

dasteg commented Nov 29, 2023

just ran into this issue while reading multiple ZipEntry from a ZipInputStream. yasson closed the stream after the first entry was read... as a workaround i read the data into byte[] and used fromJson on that. however this defeats the whole purpose of streaming

either revert this change or at least add an option to control this behavior

@jamezp
Copy link

jamezp commented Nov 29, 2023

just ran into this issue while reading multiple ZipEntry from a ZipInputStream. yasson closed the stream after the first entry was read... as a workaround i read the data into byte[] and used fromJson on that. however this defeats the whole purpose of streaming

either revert this change or at least add an option to control this behavior

Just as an FYI, what I've done is wrapped the IntputStream/OutputStream passed in in a non-closeable stream.

@marschall
Copy link
Contributor Author

Will this change by chance be reverted or partially reverted to not close the streams?

I opened #631

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

Successfully merging this pull request may close these issues.

4 participants