Added close method

[oliviercailloux]

As discussed in #108.

[rmannibucau]

Side note: can we explicit that RuntimeException can be thrown as well without being wrapped in JsonbException?
Direct implication is that JsonbException can be dropped from the signature and javadoc can surely be simplified since throwing a runtimeexception is built-in in the language.

[oliviercailloux]

Seems better to me to have a more specific kind of exception in this case. User might want to catch JsonbException and act specifically on it, but not all kinds of RuntimeExceptions.

[rmannibucau]

It is exactly the same, forcing a jsonbexception just enforces the cause to be processed but does not help to handle it in practise and does not prevent error to be thrown for example. So code will just be harder to write on user side.

Common example: catching CDI or thread pool exceptions with multi catch blocks but kot other exceptions.

[oliviercailloux]

forcing a jsonbexception just enforces the cause to be processed

What do you mean? JsonbException does extend RuntimeException, right?

[rmannibucau]

Oki, there are two point to throwing JsonbException on close:

1. pure signature point of view: it is useless or redundant with java language

2. behavior point of view: I understand the idea to say "the caller will catch only JsonbException" and it can sound sane but there are two things to keep in mind about it:
   a. it is literally the same complexity and meaning than catching RuntimeException directly so no real caller gain
   b. it makes the handling of the exception very complicated compared to native code, here is the expected handling for a caller IMHO:

   try {
   jsonb.close();
   } catch (final NoActiveContextException ne) {
   // ...ok, cdi was not active, not a big deal
   } catch (final JsonbException je) {
   // something wrong happent in jsonb impl
   } catch (final MyException je) {
   // something wrong happent in my closeable bean
   } // rethrow other exceptions

Now if you want to do the same with enforcing JsonbException then you need to catch JsonbException, call getCause() and process it using a C-style processing which sounds inaccurate for me.

Does it make more sense this way?

[oliviercailloux]

I am a bit lost in this conversation. Are you arguing for rejecting my PR? Or for modifying it? In the second case, which modification exactly so you suggest? In the first case, have you changed your mind compared to your first reply in #108, where you seemed to agree with the proposal? (Don’t get me wrong: it’s perfectly ok to change one’s mind, I just want to make sure I understand the goal of the conversation.)

[oliviercailloux]

Re-reading, I think I understand that you propose to replace throws JsonbException by throws RuntimeException. If so, I have no strong opinion either way. It anyway permits the caller to avoid having to deal with checked exceptions, which is what bothers me most.

[rmannibucau]

My request was just to remove the throws from the signature and not request JsonbException to be the only potentially thrown exception but let it be the underlying one. JsonbException can be used as an aggregator exception when mulyiple underlying ones are thrown but it does noy need to be in the sepc due to the handling issue it brings (getSuppressed). The rest of the PR is ok for me and it makes sense to move the javadoc and define that close does not throw any checked exception (no throws) IMHO.

[oliviercailloux]

Sorry, wanted to think about this then forgot to come back to it.

I still think this would be a good addition.

If I understand correctly, you argue for declaring void close(); instead of my void close() throws JsonbException;; and also, removing @throws JsonbException If any unexpected problem occurs during the close. from the Javadoc. Is this right?

[rmannibucau]

@oliviercailloux exactly yes.

[oliviercailloux]

1. I think that it’s better to keep the Javadoc @throws JsonbException If any unexpected problem occurs during the close. Here is why.

   1. It does not harm: it is implicit that any RuntimeException (or any other non checked Throwable) can still be thrown if there is some good reason not to opt for JsonbException (for example, because the problem is not related to Json, such as an InsufficientMemoryError).
   2. The documentation only (but importantly) plays a documentary and standardizing role: it announces the implementors that they are encouraged to throw subclasses of JsonbException for exceptional conditions relating to Json; rather than an implementor choosing to use JsonException, another implementor choosing to use SerializationException, without anything in common among them. Correspondingly, it permits the user to catch JsonbException when they want to catch any problems related to the Json library, in a cross-library compatible way.
   3. Catching JsonbException does not have the same behavior than catching the more general RuntimeException class.
   4. It implements Bloch’s “Item 74: Document all exceptions thrown by each method” in Effective Java (3rd ed.)

2. I also think that it’s better to keep the declaration of the exception in the header, thus void close() throws JsonbException;. Here is why.

   1. It does not force anybody to catch anything, as JsonbException is a RuntimeException.
   2. It plays a documentary role in attracting attention of the reader.
   3. It is consistent with numerous other methods in the current API, which declare JsonbException.

[rmannibucau]

@oliviercailloux issue with JsonbException is that it breaks the java exception pattern as explained before and therefore complexifies the caller code for nothing IMHO. If you write "If any unexpected problem occurs during the close" then the impl will be forced to do:

try {
    closeImpl();
}
catch (JsonbException e) { throw e; }
catch (Exception e) { throw new JsonbException(e.getMessage(), e); }

Which is exactly what i'd like we avoid due to the impact on the caller code so i'd rather document that the runtime exceptions are not wrapped in a JsonbException and that others are (rational being the close method only throws runtime exception so if it is not a runtime exception the impl must make it runtime wrapping it in a jsonb exception).

Also on keeping the runtime exception in the signature I have 2 points against it:

1. it is not the common practise in java and is kind of an anti-pattern for me (take any method you wrote recently and try to list the runtime exceptions which can be thrown, it will likely be a dozen and you didn't list them, right?),
2. it breaks frameworks/IoC interpreting exceptions there with a specific meaning (it is not highly likely we are in this case but it is not forbidden). Assume you are in an EJB for ex, the close throws JsonbException so you propagate that signature in the EJB caller for consistency, documentation etc, then you don't rollback anymore the transactions on exception which is a silent but nasty bug - and EJB are not the only ones to change the behavior if the exception is explicitly in the signature.

Hope it makes sense.

[oliviercailloux]

I am sorry, I do not understand your first point. In my mind (and in my previous comment), “the implementor” refers to the implementor of the Jsonb spec. That is the entity that provides (among others) a class that implements the Jsonb interface, thus, that has to implement the close()… (whatever is its final signature) method. Is this the “impl” that you refer to?

If so, why does it have to call a closeImpl() method that can throw JsonbException, in your example code?

If you assume that closeImpl() is some internal method designed by the implementor themselves, then I see no reason it would throw JsonbException, and in any case our discussion does not constrain internal methods of implementors such as closeImpl(), it seems to me.

When writing “as explained before”, do you refer to that comment? That one is about the caller, not implementor, and I answered that already in my previous comment.

Did I miss something?

[rmannibucau]

@oliviercailloux yes, closeImpl() was just there in pseudo code to represent the actual close implementation, the important part of the snippet was the catch blocks. Now where I'm a bit lost is when you say "I see no reason it would throw JsonbException" cause your javadoc comment enforces it if you keep it.
About the comment, you pointed out the right one, and exception definition of close is 100% about the caller and caller should be able to use standard catch blocks and not a single catch (JsonbException) and do instanceOf in this block to process actual exception. You are right Jsonb tends to explicit that but it also enforces IO exceptions and others to be wrapped which creates some exception processing issues as explained so I don't think we should do it on close which is also a place where it is not white or black cause depending the cause of the exception you can force some cleanup to release properly some resources to avoid leaks (runtime method just have the issue you don't know if it is an IO error or a processing error but it is less impacting since it does not create memory leaks).
So for me we should still drop any reference to JsonbException around close method (javadoc + signature) and we would be good.

Last thing I didn't think about before is that Jsonb alreay implements AutoCloseable since v1 so it is a breaking change to reduce the exception throwable there (existing code will potentially not compile anymore). @m0mus is it ok?

[oliviercailloux]

EDIT Forget this, I just realized that your proposal probably is that the implementor should suppress any exception occurring, and try to not throw anything (unless perhaps absolutely required), just close as best as it can, end of the story. If this is what you mean, then most of my points below are not accurate replies! (End of edit)

It seems to me that details matter here because my answer to your argument differs depending on whether you assume closeImpl() is some internal implementation method designed by some implementor, or whether your snippet really should say close() at the place it says closeImpl(). Also, I wanted to wait for clarification before answering the points you raise, but let me anyway answer them all so far as I can.

A closeImpl() internal method designed by some implementor does not have to throw JsonbException, and I expect it will not, in general. This is not contradictory to my proposal that the close() method declares and documents that it can throw JsonbException: these are two different methods. And that is the reason I do not see your point as a correct argument against my proposal. Again, I may have misunderstood something, but please be specific about the example situation you have in mind.

We agree that the “caller should be able to use standard catch blocks and not a single catch (JsonbException) and do instanceOf in this block to process actual exception”. This is permitted by my proposal. In fact, my proposal permits everything that the alternative proposal (not declaring and not documenting JsonbException throwing) permits, in terms of use cases, and more, as I stated here above. If you disagree, please provide a specific example of some code that you think my proposal will make impossible to compile or inappropriate for some reason.

You mention that it may be a problem to force wrapping IOException instances. This is however a drawback in both my proposal and the alternative ("no throw declaration") one, so can’t count as an argument in the current discussion.

About your point numbered 1. here above, this contains actually two arguments. A. It is unusual in Java. I agree, but this may not be for good reasons, and better be consistent with Jsonb itself (which declares JsonbException on most of its methods); why should suddently “consistency with the usual Java approach” be a decisive argument whereas it is not in the other methods of Jsonb? B. One should not declare all possible runtime exceptions. I agree, and this is not what I propose, see my point 1.I above. A developer (or specification designer) should decide on a case-by-case basis to document and attract attention to things that she considers are particularly worth being mentioned. For example, if IOException had been decided to be an unchecked exception from the start (an idea Brian Gosling seems to consider in retrospect would have been good), it would have been a good idea to document specifically that some methods can throw UncheckedIOException, including possibly by declaring it in their header.

About your argument 2. here above, the problem mentioned is again not different with my proposal than with the alternative. Removing the statement that close() can throw JsonbException does not solve or change anything to the fact that close(), can, in practice, throw a whole bunch of things, depending on the underlying implementation. Not documenting it does not make the problem go away. Also, the contract of any dependent entity (such as an EJB) does not necessarily have to declare the throw in turn: a less specific class can always choose to be more vague than its more specific descendants, as the classical O-O approach suggests. Thus, EJB will only know that it can throw RuntimeException (and this need not be declared specifically, following the usual rule that only non-obvious things deserve being mentioned; this is in any case considered implicit on any method).

[oliviercailloux]

It springs to my mind that your last point might be about comparing not “my” version to “your” version, but comparing to the existing state of fact. Indeed, constraining the close() method in any possible way is a breaking change, whether with “my” version (declaring JsonbException) or “your” version (not declaring any throw). That is because the previous version keeps the declaration throws Exception of the AutoCloseable interface. You had noticed it already in the related issue, and wrote “Sounds reasonable to me to use JsonbException and only impacts implementations in terms of breaking change so it looks "ok"”, so I thought this was out of the discussion.

If it is considered absolutely forbidden to break this, then it is impossible to achieve, in any possible form, what (I think) we both thought would be good to have, namely, not having to catch Exception when using a try-with-resources with Jsonb. This would be sad, and I hope a breaking change (on the implementation side) can be tolerated for this case, because this is a major pain when using the API.

[rmannibucau]

Ok, so - assuming we can do this little breakage cause we are going 2.0 and waiting for m0mus feedback, here are the points trying to simplify the discussion:

1. jsonb pattern is to declare JsonbException (which is by itself a small drawback with some framework but not a big one IMHO) + enforce to throw this (which is a blocker for me and what we discuss mainly). This point can lead (assuming once again we can do small breakages) to remove it from all methods in another ticket/pr (cc @m0mus).
2. close() method releases any Jsonb components (adapters, serializers, ...) and internals. here all types of exceptions are accepted and we have multiple strategies:
   2.a. enforce jsonbException to wrap any exception and fail at first exception
   2.b. enforce jsonbException to suppress (jsonbEx.addSuppressed()) any exception and fail at the end (enables to clean all underlying components)
   2.c. just let the exception go through and let the caller handle it as desired/usual
   2.d. 2.b but with another type (you will understand later why I mention it)
   2.e. 2.b if there are 2 exceptions otherwise throw the unique exception

In terms of impact in the API, none require to declare JsonbException in the signature (BTW IDE tend to recommend to drop it from the signature these days it seems) but I can leave with 1 decision there. What is the most important is what the behavior of the method should be, i.e. how the caller can interact with errors.

Currently close behavior is defined by AutoCloseable (except for jsonb instance access point which is not the point of this thread) which does not define any real behavior. This means we can pick any impl we want. To ensure we don't break too much users I checked out both main impls (yasson and johnzon) and sadly the impl differs. Yasson let the first exception be thrown without wrapping it (2.c) and johnzon aggregates exceptions in an IllegalStateException (mainly because the close is handled by a mapper under jsonb layer). So concretely a johnzon user will catch IllegalStateException but all components had been released and a yasson user can catch anything and components can leak.

With that state in mind I think we need to fix that anyway and I wonder what about a 2.e proposal:

I. I think it is important to release anything so principal would be 2.b as basis
II. but it is also very important to handle exceptions easily so here I see two easy and efficient options:
II.a. add in properties a Consumer<T extends Exception> which can see all exception during close but must not throw any (enables to respect 1 and after close to handle it). The jsonb implementation would extract T to mimic catch behavior (by type) so it would be close to adapters in terms of type behavior.
II.b. throw a JsonbAggregatedException (extends JsonbException) which adds to JsonbException a <E extends Exception> Optional<E> getException(Class<E>) and enables to process exceptions in a not "java" way but still easy way.
II.c. mix 2.a and 2.b (ie add on JsonbAggregatedException a ` void on(Class, Consumer)).

It is not as smooth as I expected but I suspect the "aggregator" point kind of kill the "let it go" wish.

Side note: if going with a JsonbAggregatedException option, i'd like the impl and exception matching part stays in the impl and not API so it probably requires to go through JsonbProvider.

So personnally (and a bit sad) I think I prefer 2.b+II.b options to avoid leaks.

Hope it makes sense.

[oliviercailloux]

Thanks for all the discussion.

At this point it appears to me that there are multiple decisions to be taken from a high-level design point of view of the Jsonb specification, which I am not qualified to discuss in any depth, being not sufficiently involved in that specification or in other related discussions.

It still seems to me that my arguments (summarized here above as 1.I to 2. III) show a superiority of declaring throws JsonbException and documenting that exception in the Javadoc. But at this stage I suppose the discussion will not progress easily, and better wait for other high-level decisions to be taken, so I suppose I’ll leave it here.

I am ready to modify my PR when you decide on anything, hoping that at least the most important issue (not throwing Exception on close()) can be solved.

[oliviercailloux]

Time has passed and I believe that we still agree that the status quo is unsatifactory: the spec does not indicate precisely enough the behavior of close() wrt exceptions and so users can’t easily write portable code.

Re-reading the whole thread again, here is a summary of my current proposal integrating some of your suggestions. I focus on the behavior and leave the choice of the method signature for a second stage.

I propose to declare an explicit method close() in Jsonb so as to mandate close() to throw only unchecked exceptions.

The javadoc or other specification document should indicate somehow that implementors are encouraged to throw only JsonbException or subclasses of that exception when their own code creates the exception. Some of these subclasses of JsonbException could be standard and implementors could add more if they so desire, thereby creating a hierarchy similar to ProcessingException. When the exception is created not by their own code but by user code, for example, if closing user provided CDI beans produces user instantiated unchecked exceptions, then they should be thrown as-is. If calling some user code that throws checked exceptions (assuming this is possible), these exceptions should be wrapped into a JsonbException or a subclass of it.

I’d be happy to get your thoughts about other cases such as ContextNotActiveException instances.

The spec should also indicate to implementors that if closing produces exceptions, the rest of the closing process should be conducted as best as possible, and further exceptions must be recorded as suppressed exceptions. That’s the solution 2b in your comment. The part II seems unnecessarily complicated to me: the suppressed exception mechanism exists in Java exactly for this use case and developers should be used to it.

[oliviercailloux]

BTW I guess that this proposal is in view of 4.0, as this involves some breaking change for implementors.