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

Investigate wrapper exceptions #346

Closed
cruftex opened this issue May 15, 2016 · 1 comment
Closed

Investigate wrapper exceptions #346

cruftex opened this issue May 15, 2016 · 1 comment
Assignees
Labels
Milestone

Comments

@cruftex
Copy link
Member

cruftex commented May 15, 2016

The Spec says:

There is also a blocking implementation of CompletionListener, CompletionListenerFuture.
It implements both CompletionListener and Future. If the onException(Exception e)
method of CompletionListener is called, the exception is wrapped in ExecutionException and
rethrown by the Future get() and get(long timeout, TimeUnit unit) methods.

But the code, does not wrap:

  public void onException(Exception e) throws IllegalStateException {
    synchronized (this) {
      if (isCompleted) {
        throw new IllegalStateException("Attempted to use a CompletionListenerFuture instance more than once");
      } else {
        isCompleted = true;
        exception = e;
        notify();
      }
    }
  }

ExecutionException means probably java.util.concurrent.ExecutionException. Should be linked
or fully qualified if exceptions are mentioned not in the scope of javax.cache. Shouldn't it be a CacheLoaderException?!

TODO:

  • investigate wrapper usage
  • some general statement about customizations and wrapper should be in the spec

Releated:
jsr107/jsr107tck#99
jsr107/jsr107tck#85

We need probably more specific issues on what actually needs change. Any more findings and suggestions are appreciated.

Relevant tests:

  • org.jsr107.tck.integration.CacheLoaderTest
  • org.jsr107.tck.integration.CacheWriterTest
@cruftex
Copy link
Member Author

cruftex commented Jun 10, 2016

Regarding the loadAll() the remarks from @christian-esken:

org.jsr107.tck.integration.CacheLoaderTest

// Observation: shouldPropagateExceptionUsingLoadAll() requires a CacheLoaderException

// Issue: Javadocs do not mandate this for Cache.loadAll().
//    a) Javadoc states: "If a problem is
//     encountered during the retrieving or loading of the objects,
//     an exception is provided to the {@link CompletionListener}."
//     b) The @throws declaration is "@throws CacheException        thrown if there is a problem performing the load."

// Proposed change: Change CacheLoaderTest to check for CacheException

The relevant code is

    CompletionListenerFuture future = new CompletionListenerFuture();
    cache.loadAll(keys, false, future);

    //wait for the load to complete
    try{
      future.get();
    } catch (ExecutionException e) {
      assertThat(e.getCause(), instanceOf(CacheLoaderException.class));
    }

OTOH CompletionListenerFuture does correctly document that it wraps with ExecutionException on the method get(), opposing my initial statement.

Re-evaluating everything I think the current checks and Spec is consistent and useful as well.

  1. The immediate exception is the ExceutionException as documented
  2. The checks that the cause is CacheLoaderException which is correct, too, since an exception originated from the loader

Effectively in this case the original cause is double wrapped. Once by the CacheLoaderException, and again by the ExecutionException. That's suprising, but the only way to fulfill the future interface contract.

BTW: There is a mistake in the test not including a fail(). Opening a TCK issue for that.

@cruftex cruftex closed this as completed Jun 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant