TCK assertions wrong for Cache.invoke and wrapped exceptions

[cruftex]

TCK challange for:

• CacheInvokeTest.noValueException()
• CacheInvokeTest.removeException()
• CacheInvokeTest.existingException()

The TCK entry processor in these tests produces the following exception:

javax.cache.processor.EntryProcessorException: java.lang.IllegalAccessError

and asserts that an exception will be thrown by invoke with cause java.lang.IllegalAccessError e.g. by:

try {
  cache.invoke(key, new ThrowExceptionEntryProcessor<Integer, String, Void>(IllegalAccessError.class));
  fail();
} catch (CacheException e) {
  assertTrue("expected IllegalAccessError; observed " + e.getCause(),
      e.getCause() instanceof IllegalAccessError);
}

The EntryProcessorException documentation states:

An implementation must wrap any {@link Exception} thrown by an
{@link EntryProcessor} in this exception.

This also applies to this type of exception itself.

Is the Spec enforcing that an EntryProcessorException must be passed on directly?

Proposed changes:

Change the TCK to test that an exception is wrapped by the cache implementation.

Change the TCK not to expect that the original exception is the immediate cause, but the root cause.

Any thoughts?

[christian-esken]

I am currently verifying tCache from https://github.com/trivago/triava against the TCK and also saw the issue. I agree that it is a bug in the TCK, and also fully support the proposed changes.

The other possible way would be to mandate that any "EntryProcessorException must be passed on directly". I would not like that, as it would be surprising and error-prone to offload the correct unwrapping to the invoke() caller. The caller should get the Exception as-is.

[christian-esken]

+1

[cruftex]

Just removed the idea to change the exception in the API for the EntreProcessor.process method:

T process(MutableEntry<K, V> entry, Object... arguments)
  throws EntryProcessorException;

That is not strictly binary compatible and might break things. We cannot fix the interface.

[cruftex]

There are a couple of other places that specify exceptions such as CacheLoaderException, CacheWriterException, CacheEntryListenerException. There seems to be the same problem: The application code can throw the exception and it should be used by the cache implementation to wrap an application exception.

The TCK tests that an Exception is not wrapped by the implementation when it is already a CacheException. I cannot find this behavior in the spec. With respect to the CacheWriterException the rationale behind it is discussed in jsr107/jsr107spec#224

Re-Throwing an exception directly will lead to confusion if the code was run in another thread or on another JVM. In this case the stack trace is wrong and would not include the actual caller. IMHO cache implementations should consistently wrap exceptions, the stack trace of the wrapped exception must fit the actual calling context.

For a 1.1 MR I would suggest to relax the TCK tests, that implementations may be allowed to consistently wrap exceptions. Thoughts?

[cruftex]

Wrap up of possible changes:

a) do nothing

b) Allow cache implementations to always wrap the exception, as the Spec says, but also allow the current behavior of not wrapping, if it is already a CacheException. This means that existing implementations don't need to change, but can do the agreed correct thing and always wrap.

c) enforce that cache implementations always wrap the thrown exception. Cache implementation need to change the behavior.

@vbekiaris and @cruftex discussed this and agreed to proceed with plan b.

[cruftex]

I added a more thorough explanation and alaysis in a spec issue: jsr107/jsr107spec#388.
Ideally the API should be changed, which is not possible for 1.1

[cruftex]

Closed with #131