java8: dealing with default methods

Question

While writing a crypto utility class I ran into a problem with the following method:

public static void destroy(Key key) throws DestroyFailedException {
    if(Destroyable.class.isInstance(key)) {
        ((Destroyable)key).destroy();
    }
}

@Test
public void destroySecretKeySpec() {
    byte[] rawKey = new byte[32];
    new SecureRandom().nextBytes(rawKey);
    try {
        destroy(new SecretKeySpec(rawKey , "AES"));
    } catch(DestroyFailedException e) {
        Assert.fail();
    }
}

In the particular case of javax.crypto.spec.SecretKeySpec the above method would work just fine in java7 because SecretKeySpec (javadocs 7) does not implement Destroyable (javadocs 7)

Now with java8 the class SecretKeySpec (javadocs 8) has been made Destroyable (javadocs 8) and the method Destroyable#destroy is now default which is fine according this statement

Default methods enable you to add new functionality to the interfaces of your libraries and ensure binary compatibility with code written for older versions of those interfaces.

then the code compiles without any problems despite the fact that the class ScretKeySpec itself has not been changed, alone the interface SecretKey has been.

The problem is that in oracle's jdk8 the destroy method has the following implementation:

public default void destroy() throws DestroyFailedException {
    throw new DestroyFailedException();
}

which leads to an exception at run time.

So the binary compatibility might not have been broken, but existing code has been. The test above passes with java7 but does not with java8

So my questions are:

• How to deal in general with default methods which might lead to exceptions - because not implemented or not supported - or unexpected behavior at run time? aside from doing

  Method method = key.getClass().getMethod("destroy");
  if(! method.isDefault()) {
      ((Destroyable)key).destroy();
  }
  

  which is only valid for java8 and which might not be correct in future releases, since the default method might get a meaningful implementation.

• Would it not be better to leave this default method empty instead of throwing an exception (which IMO is misleading since aside from the legit call to destroy nothing has been attempted to effectively destroy the key, an UnsupportedOperationException would have been a better fit and you would know instantly what is going on)

• Is my approach with (type check/cast/call)

  if(Destroyable.class.isInstance(key))
      ((Destroyable)key).destroy();
  

  for determining whether to destroy or not wrong? What would be an alternative?

• Is this a misconception or did they just forget to add meaningful implementation in ScretKeySpec?

Answer 1

Is this a misconception or did they just forget to add meaningful implementation in SecretKeySpec?

Well they didn't forget. SecretKeySpec does need an implementation, but it simply hasn't been done yet. See bug JDK-8008795. Sorry, no ETA on when this will be fixed.

Ideally, valid implementations of destroy would have been added at the time the default method was added and the interface was retrofitted onto existing classes, but it didn't happen, probably because of scheduling.

The notion of "binary compatibility" in the tutorial you cited is a rather strict definition, which is the one used by the Java Language Specification, chapter 13. Basically it's about valid transformations to library classes that do not cause class loading or linking errors at runtime, when combined with classes compiled against older versions of those library classes. This is in contrast to source incompatibility, which causes compile-time errors, and behavioral incompatibility, which causes usually unwanted changes in the runtime behavior of the system. Such as throwing exceptions that weren't thrown before.

This is not to minimize the fact that your code got broken. It's still an incompatibility. (Sorry.)

As a workaround, you might add instanceof PrivateKey || instanceof SecretKey (since these are apparently the classes that lack destroy implementations) and have the test assert that they do throw DestroyFailedException, else if instanceof Destroyable execute the remainder of the logic in your test. The test will fail again when these instances get reasonable destroy implementations in some future version of Java; this will be a signal to change the test back to calling destroy on all Destroyables. (An alternative might be to ignore these classes entirely, but then valid code paths might end up remaining uncovered for quite some time.)

Answer 2

I'm only speculating, but I think the idea behind throwing an exception in the default implementation of destroy, is to alert you that the sensitive data wasn't destroyed. If the default implementation was empty, and there's no implementation overriding the default one, you might assume by mistake that the sensitive data was destroyed.

I think you should catch the DestroyFailedException exception anyway, regardless of whether it is thrown from the default implementation or from a real implementation, since it warns you that nothing was destroyed, and you should decide how to handle this situation.

The contract of the destroy method, which hasn't changed between Java 7 and Java 8 (aside from the comment about the default implementation) says - Sensitive information associated with this Object is destroyed or cleared. Subsequent calls to certain methods on this Object will result in an IllegalStateException being thrown.

and :

Throws:
DestroyFailedException - if the destroy operation fails.

If destroy fails, subsequent calls to certain methods on this Object will not result in an IllegalStateException being thrown. That's still true if destroyed did nothing, and therefore the default implementation (which does nothing) throws DestroyFailedException.