search

eclipse-ee4j / yasson

Eclipse Yasson project
https://projects.eclipse.org/projects/ee4j.yasson
Other
204 stars 96 forks source link

Close Parsers and Generators #586

Closed marschall closed 1 year ago

marschall commented 1 year ago

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

jamezp commented 1 year ago

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 commented 1 year ago

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

marschall commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

marschall commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

I opened #631