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

Customizations may implement Closeable - TCK tests missing #100

Closed
cruftex opened this Issue May 14, 2016 · 4 comments

Comments

Projects
None yet
1 participant
@cruftex
Member

cruftex commented May 14, 2016

On the Java Doc of Cache.close() this is specified:

This includes calling the close method on configured {@link CacheLoader},
{@link CacheWriter}, registered {@link CacheEntryListener}s and
{@link ExpiryPolicy} instances that implement the java.io.Closeable
interface.

I do have a 100% TCK compatible implementation that is not supporting this.

Resource management and lifecycle is an important issue. A TCK test should be added.

@cruftex

This comment has been minimized.

Member

cruftex commented Jun 6, 2016

All customization implementations in the TCK depend on CacheClient which implements AutoCloseable (This is Java 1.7) but not Closeable. Indeed, in the close() method the open connections are closed.

It seems that the tear down code of the various tests in the TCK is fairly consistent. It first closes the cache and after that stops the server part for the customization. E.g. the ExpiryPolicy tear down is:

  @After
  public void cleanupAfterEachTest() throws InterruptedException {
    for (String cacheName : getCacheManager().getCacheNames()) {
      getCacheManager().destroyCache(cacheName);
    }
    expiryPolicyServer.close();
    expiryPolicyServer = null;

    //close the server
    cacheEntryListenerServer.close();
    cacheEntryListenerServer = null;
  }

=> The lifecycle of a cache is managed properly, but the close method of the customization
will never be called, since AutoCloseable is the wrong interface.

This means the following change would make sense:

  • Implement Closeable
  • Assert on CacheServer.close() that all client connections have been closed

This way we check consistently that all cache implementations call close() correctly.
The existing code seems to anticipate this already.

For distributed environments there may be arguments for:

  • Closing of customization on remote systems may lag, since: The system may not be reachable, or
    the application should not be held up in general
  • Lifecycle of customizations are independent of application lifecycle, since the cache lifecylce
    is actually, too. See Gregs' comment at jsr107/jsr107spec#316 (comment)

All in all, I think it is safe and necessary to assume that after the last connected application closed
a cache that its customization should be closed (and unloaded), too. If not, that would lead to terrible versioning conflics.

@cruftex cruftex self-assigned this Jun 8, 2016

@cruftex cruftex added this to the 1.1.0 milestone Jun 8, 2016

@cruftex cruftex changed the title from Customizations may implement Closable - TCK tests missing to Customizations may implement Closeable - TCK tests missing Jun 8, 2016

@cruftex

This comment has been minimized.

Member

cruftex commented Jun 8, 2016

The approach seems viable. Current findings:

  • When requireing that "Customization.close()" is called before the server is closed, only three of the existing TCK tests fail, since they use an extra server that is closed before the cache is destoryed
  • The RI is implementing the spec'd Closeable semantic
  • The RI has a bug yielding a ClassCastException, when an EntryListener implements Closeable
@cruftex

This comment has been minimized.

Member

cruftex commented Jun 8, 2016

I verified the change with the jsr107-test-zoo, to see how it affects existing implementations:

  • EHCache 3 and hazelcast 3.4 work according to the Spec and pass the updated TCK
  • Oracle Coherence works according to the Spec, except that it has the same bug as the RI. Using Closable with EventListeners yields a ClassCastException

All other tested implementations (do not support calling Closeable.close() for all customizations.

Decision:

We could drop the requirement from the Spec or we can extend the TCK to test it correctly. Since it is implemented by the RI and some of the existing implementations, I vote for extending the TCK.

@cruftex

This comment has been minimized.

Member

cruftex commented Jun 8, 2016

Pull request for RI is ready, fixing the ClassCastException
Pull request for TCK is ready.

Outline of the change

All TCK customizations implment now Closeable.

The client/server code in the TCK is extended so the client (which is the instantiated customization by the cache implementation) sends a close operation to the server, when the close() method is called on it.

The server checks that all client connections are closed correctly when it receives the close. If this is not the case an IllegalStateException is thrown.

Since this is a cross cutting concern there is no extra test that checks the semantics. It is checked all the time an required by the client/server code now. If a cache is not implementing Closeable support, all tests addressing loaders, writers, expiry policy and entry listeners will therefore fail. Some more rationale: see second comment above.

In case the customization is not closed a warning is logged with the open connections and an exception is thrown:

Jun 08, 2016 6:42:07 PM org.jsr107.tck.support.Server close
WARNING: Open client connections: {0=Thread[Thread-2,5,main]}

java.lang.IllegalStateException: Excepting no open client connections. Customizations implementing Closeable need to be closed. See https://github.com/jsr107/jsr107tck/issues/100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment