Close throws Exception

[oliviercailloux]

AutoCloseable::close documentation mentions: “While this interface method is declared to throw Exception, implementers are strongly encouraged to declare concrete implementations of the close method to throw more specific exceptions, or to throw no exception at all if the close operation cannot fail.”

Would you consider modifying the declaration of close() in Jsonb::close (currently not declared thus inheriting from AutoCloseable) as throwing JsonbException?

Relatedly, the documentation of Jsonb mentions: “Calling Closable.close() method will cleanup all CDI managed components (such as adapters with CDI dependencies) created during interaction with Jsonb.” Can I conclude that, if using Jsonb in Java SE (and no CDI), it is safe to not close the jsonb instances I create? If so, would you consider documenting that fact?

[rmannibucau]

Sounds reasonable to me to use JsonbException and only impacts implementations in terms of breaking change so it looks "ok".

Regarding your second question: implementations can use this method for anything, CDI being the most common case and default one only so better to always call it IMHO.

[oliviercailloux]

I understand that currently, the spec allows for implementers to leave “resources” open during the lifetime of a jsonb instance and expect that the user will close the jsonb instance. And therefore, the user has to close the jsonb instances.

I wonder if a change in the spec could be considered: giving one more guarantee to the user, namely, that the jsonb instance does not have to be closed in non CDI-environments.

I realize that, as usual, giving supplementary guarantees to the user in the spec comes with the price of a supplementary constraint to implementors. And therefore, in general, possible lowering of quality of service or difficulty in ensuring that the implementation is correct.

But in this case, having to close instances is a burden to the user that I suspect is not justified by related improvements in terms of implementation performance or ease of implementation. That is because I hardly see why an implementor would significantly benefit from being able to use system resources or anything similar that needs to be closed when the work with a jsonb instance is done.

I could be wrong of course, even more so that I am not an implementor of the jsonb spec.

In any case, making close throw a specific exception would already be a great improvement, IMHO.

[rmannibucau]

that the jsonb instance does not have to be closed in non CDI-environments.

You can't assume that by a lot of integration - any IoC integration will require that constraint, some memory optimization will require it as well, some GC enablement require it even in plain SE too.

Note that the autocloseable usage makes this "burden" very low in all trivial SE cases and in all other cases you have a kind of lifecycle where close is mandatory so it is good to keep it IMHO and less misleading for libraries and users than putting some rules like "if you don't use anything you can skip this method" IMHO .

On the exception side if I get it right your request is not a JsonbException but a RuntimeException to not catch it - just to clarify the goal. I agree with you that being able to use method reference would be nice and this does not break user code to do it as you propose so +1.

[oliviercailloux]

You can't assume that by a lot of integration - any IoC integration will require that constraint, some memory optimization will require it as well, some GC enablement require it even in plain SE too.

Well, I am happy to leave the last word to you on that matter. But just for my own curiosity, I must say I do not understand the argument, because I do not see anything specific to JSON-B that justifies that it must be closed, at least if we exclude CDI related matters (and see below about CDI). By that reasoning, if I understand it correctly, it seems like any library providing any kind of service should require the user to close it when finished. Fortunately, this principle does not seem to be usually followed. Consider JAXB as an example. Although it is a complex spec that involves a great variety of complex objects and memory burden in general, as far as I recall, (almost?) nothing must be closed by the user when using JAXB. See the various examples in Unmarshaller.

And now that I think about it, even when using CDI injected managed beans, I am not sure I understand the requirement to close jsonb instances. CDI already defines the lifecycle of the managed beans, and thus it is already specified when to stop their life (for example, at the end of a session for a @SessionScoped bean). Won’t this conflict with the requirement of stopping their life when a jsonb instance is closed (if this is what the documentation in the close method suggests)?

less misleading for libraries and users than putting some rules like "if you don't use anything you can skip this method" IMHO

I suppose there is some subjectivity here. It does not hurt me as it is common in the Java world. AutoCloseable mentions: “It is possible, and in fact common, for a base class to implement AutoCloseable even though not all of its subclasses or instances will hold releasable resources. (…) when using facilities such as Stream that support both I/O-based and non-I/O-based forms, try-with-resources blocks are in general unnecessary when using non-I/O-based forms.” And indeed, one can see plenty of code around using streams without closing them, and I am sure we will agree that this is correct use. This kind of case is actually so common (even beyond streams) that Eclipse integrates it in its analysis.

On the exception side if I get it right your request is not a JsonbException but a RuntimeException to not catch it

Not sure I understand what you mean? JsonbException is a RuntimeException. In any case, if sticking to the close mechanism, I’d vote in favor of declaring a JsonbException on close() for uniformity with the rest of the exceptions being thrown by jsonb rather than creating another one for this purpose. (The default idiom that I use when dealing with jsonb is exemplified here.)

[rmannibucau]

@oliviercailloux JAXB does not have very advanced implementations so the memory and so on issue does not appear AFAIK. However the issue exists trivially if you use CDI - or any other resource related impl - in adapters and yes it leaks today without workarounds in apps.

The CDI requirement is when the instances are @dependent, JSON-B will create a creational context and will need to release it when no more needed - to destroy transitive dependent beans etc. In general JSON-B must provide a hook to release all needed resources used by user instances during the runtime. CDI is just here by default and can be implicit which is why a library can't bypass close if it aims to be used in any CDI/EE container.

The point about the exception is that JsonbException is not that important since when call close you catch or not the exception but you don't have any other options so whatever the type is it will match your original need which is to not catch it in general to meet java method reference syntax. Just wanted to highlight that point cause I think it is the way to express it in the spec rather than just saying it is a JsonbException so maybe "close method is overriden to throw a RuntimeException of type JsonbException so if you want to handle potential errors you need to catch it yourself but it enables you to use method reference to call it".

Do you want to try to do a PR on the spec in that direction?

[oliviercailloux]

(The CDI spec says: “Finally, the container is permitted to destroy any @dependent scoped contextual instance at any time if the instance is no longer referenced by the application”. I’d have to think more about all this… And I am still slightly worried that this reasoning seems to apply to everything, and thus seems to lead to having to systematically close everything we use that is provided by any library.)

But let’s cut the discussion short, in any case this was not the main point of this issue. (Thanks for your patience anyway.)

Yes, to summarize, I think we agree that overriding the close method in the Jsonb interface to throw a more specific exception, namely one of type JsonbException, which is a RuntimeException, would be an improvement.

I am sorry, what is a PR and how should I proceed?

[rmannibucau]

Yes but permitted != does ;)

A PR = pull request. You "fork" the spec repository to get your own copy, do the change then hit "create a pull request" and follow the form github will propose you. It will propose the change to the spec and make easier/concrete the review compared to a discussion thread.

[oliviercailloux]

Ah, yes, I know what a pull request is, it just didn’t occur to me that PR was referring to that in this context.

Done. (As you’ve seen already.)

[aguibert]

Closing this issue as it appears to be complete

[oliviercailloux]

The corresponding PR (#109) is stalled since a long time. Anyway, I have re-read the whole thread and thought again about this issue. I hope it is appropriate to come back to it here (please tell me if I should rather open a new issue).

I’d like to insist with my proposition that the spec be slightly changed to mention that it is safe to omit calling close() when the user does not use adapters, serializers or deserializers that use CDI injection of beans under a “dependent” scope. Here is my justification for this proposition.

1. The only explicit justification for users having to close their Jsonb instance that I can find in the current spec is that Jsonb may instanciate CDI beans for injecting into adapters or serializers (see sections Adapters and Serializers/Deserializers).
2. Adapters, serializers and deserializers have to be explicitly registered when creating a Jsonb instance (on the builder), or are recognized thanks to annotations on class definitions. In the first case, instances are given to the JsonbBuilder, so Cdi is possibly involved only in the second case. Thus, in many cases, the user of Jsonb will be able to determine with certainty that she needs no injection facility (e.g., the user uses Jsonb only to deserialize a few types of her own design, using only adapters of her own design, that do not @Inject anything).
3. Mentioning that it is safe to omit calling close() in such situations will alleviate a significant burden from the user of Jsonb.
4. There might be an implicit justification for users having to close their Jsonb instance. Omitting to mention that it is safe to omit calling close() in such situations might, in principle, introduce some opportunities for implementors, who will be able (at least, normatively speaking) to count on users closing Jsonb instances. That is a point that has been raised here above. But, a) a supplementary burden should be imposed to users only if it is clear that it permits significant gains on the implementation side; b) no proof, not even a clear clue, has been given that such opportunities really exist; c) Jackson and Google Gson appear to achieve very good performances without such a burden.

I note that rmannibucau disagrees with some of the above points.

You can't assume that by a lot of integration - any IoC integration will require that constraint, some memory optimization will require it as well, some GC enablement require it even in plain SE too.

I read this point as arguing against my item 4. But the point seems too vague to justify a requirement to close instances on the part of the user, and is at least partly countered by 4. c). A specific example would certainly help, if possible.

Note that the autocloseable usage makes this "burden" very low in all trivial SE cases

This is probably subjective, but strikes me as a very unconsensual statement. Even with the modern try-with-resources syntax, having to close instances explicitly rather than let them be garbage collected automatically is a pain.

I hope that my proposition can be seriously considered, even if rmannibucau still disagrees with some of the more subjective aspects of the discussion. I think it would help Json B gain traction in the Java SE world. (Which I would consider nice because, first, a standard with a spec is better than multiple incompatible implementations; and second, after having used both Jackson and Gson, I find the Json B approach more elegant on multiple aspects.)

[oliviercailloux]

Closing this issue as it appears to be complete

It is unclear to me why this issue has been closed, as it is not complete. Should I open a new one?

[rmannibucau]

Not sure why it was closed but to comment why close must be called (your previous comment) here are some inline inputs:

it is safe to omit calling close() when the user does not use adapters, serializers or deserializers that use CDI injection of beans under a “dependent” scope.

No, as soon as you create a JSON-B instance, even without any CDI instance nor IoC you have to call close because the impl can require it (free() like call if not using "new" equivalent is a good example).

The only explicit justification for users having to close their Jsonb instance that I can find in the current spec is that Jsonb may instanciate CDI beans for injecting into adapters or serializers (see sections Adapters and Serializers/Deserializers).

Don't forget the bean can be a CDI bean without any injection (but to use the scope or be injected itself in another bean which is not JSON-B related), this is per spec as of today - but once again close() is used by CDI case but is way more generic so don't think using CDI as a justification will lead anywhere :s.

Adapters, serializers and deserializers have to be explicitly registered when creating a Jsonb instance (on the builder), or are recognized thanks to annotations on class definitions. In the first case, instances are given to the JsonbBuilder, so Cdi is possibly involved only in the second case. Thus, in many cases, the user of Jsonb will be able to determine with certainty that she needs no injection facility (e.g., the user uses Jsonb only to deserialize a few types of her own design, using only adapters of her own design, that do not @Inject anything).

This statement is right but unrelated to close() IMHO.

Mentioning that it is safe to omit calling close() in such situations will alleviate a significant burden from the user of Jsonb.
There might be an implicit justification for users having to close their Jsonb instance. Omitting to mention that it is safe to omit calling close() in such situations might, in principle, introduce some opportunities for implementors, who will be able (at least, normatively speaking) to count on users closing Jsonb instances. That is a point that has been raised here above. But, a) a supplementary burden should be imposed to users only if it is clear that it permits significant gains on the implementation side; b) no proof, not even a clear clue, has been given that such opportunities really exist;

You can omit it if you don't write a portable code at all and know your implementation in the version you run.
The goal of JSON-B is to enable portable code to be written and there close must be used.
In terms of burden it is mainly a try with resource or a @PreDestroy in all well known IoC so not much compared to the guarantees it gives and freedom to the impl it enables.

c) Jackson and Google Gson appear to achieve very good performances without such a burden.

Depends, this is generally not true but to be concrete the case you reference there works because it uses ThreadLocal instead of references behind the mapper and it does leak in several environment so even for them it is better to clean up the instance properly and configure them accordingly.
So long story short: they need a close callback to be used (thinking to OSGi and EE cases) if you don't want to have surprises with some JVM/servers - and they don't even use the advanced strategies I referenced at the beginning.

[oliviercailloux]

even without any CDI instance nor IoC you have to call close because the impl can require it
I believe that this is the main issue of this discussion.

free() like call if not using "new" equivalent is a good example

Can you please give me a concrete example of what you mean? Or an article that discusses this? Or perhaps, is there a current implementation of JSON-B that does release some resources not related to IoC when calling close(), with some documentation about it?

[rmannibucau]

@oliviercailloux not sure there are much public cases but here are the cases I encountered - but once again the API to be portable needs it even without a concrete case:

1. All impl to be "fast" use some cached buffers for I/O (preallocated arrays), in johnzon you can use the "cache" you want (from a queue to be portable to a custom one without forgetting the well known thread local ;)). There are some use cases to plug a custom impl there where you use C primitives - not malloc/free but more advanced/specific one - to allocate the "memory", here you need a close callback to cleanup the allocated resources.
2. You said standalone case does not care about close but some adapters/(de)serializer can implement AutoCloseable and JSON-B should (must in theory) respect that when the instance is fully owned (newInstance) by JSON-B - common case for me is to lookup some mapping in a mapped file.
3. JSON-B is free to implement some synchronization mecanism to await pending writes before close exist and avoid to make pending read/write failling (gracefull shutdown case)

Indeed, close does not define anything so most of these use cases are not portable as of today but since JSON-B defines by spec that close call is required then they are all enabled nicely.
Concretely, if you want to define all cases close must handle, it will be very difficult to keep the spec understandable and light.
The best you can get is that "it is highly probable that you can skip close if you don't use any other API than Jsonb instance" but even with this limitation you can encounter cases you want to call close so I wouldn't try to define these cases which will only lead to a wrong, and against the spec intent, usage at the end.

[oliviercailloux]

Thank you for these clear and concrete examples.

Concretely, if you want to define all cases close must handle, it will be very difficult to keep the spec understandable and light.

I agree that such approach would make the spec too fuzzy and would be undesirable. So I no longer wish to remove the requirement for closing altogether. (However I still suspect that a better exception management is desirable, which was the initial point of this thread on which we got stuck some time ago. I’ll come back to it soon if I have not exhausted your patience.)

[rmannibucau]

However I still suspect that a better exception management is desirable

Still agree but we need to take care of the backward compatibility, we had some bad time changing some internal exception stacks in johnzon even if we made it (and exception stack are not validated by static tools at all so it is can be silent breaking change at build time).

[oliviercailloux]

I just commented at #110: I thought that it’s best to comment there as the more technical discussion happened there already. Please tell me if this is inappropriate.