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