Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCK assertions wrong for Cache.invoke and wrapped exceptions #85

Closed
cruftex opened this issue Jan 26, 2016 · 7 comments
Closed

TCK assertions wrong for Cache.invoke and wrapped exceptions #85

cruftex opened this issue Jan 26, 2016 · 7 comments
Assignees
Labels
Milestone

Comments

@cruftex
Copy link
Member

@cruftex cruftex commented Jan 26, 2016

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
Copy link

@christian-esken christian-esken commented May 3, 2016

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
Copy link

@christian-esken christian-esken commented May 3, 2016

+1

@cruftex
Copy link
Member Author

@cruftex cruftex commented Dec 5, 2016

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
Copy link
Member Author

@cruftex cruftex commented Oct 7, 2017

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 cruftex self-assigned this Oct 7, 2017
@cruftex
Copy link
Member Author

@cruftex cruftex commented Oct 7, 2017

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
Copy link
Member Author

@cruftex cruftex commented Oct 17, 2017

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
Copy link
Member Author

@cruftex cruftex commented Oct 17, 2017

Closed with #131

@cruftex cruftex closed this Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.