Closeable ContentSigner

[artem-smotrakov]

ContentSigner objects hold an output stream that may need to be closed to prevent resource leaks. A caller can get the stream by calling getOutputStream() method, and close it once the work is done. However, it may be easy to forget to do that.

If ContentSigner extended AutoCloseable:

• a caller could use it in a try-with-resources block
• static analysis tools could detect places where a ContentSigner.close() is not called

Updating ContentSigner to extend AutoCloseable is definitely an update in the public API. That may potentially result to compilation errors in client apps. Those errors should be easy to fix thought. Moreover, those errors may make developers look at and fix possible resource leaks in their code.

Here is a summary of the update:

• Updated interface ContentSigner to extend AutoCloseable.
• Implemented close() method in several anonymous classes that implement ContentSigner.

[dghgit]

I'm not sure... was there a specific reason you did not suggest changing the return for getOutputStream()?

[artem-smotrakov]

Do you mean changing the return type of getOutputStream()? I don't think it's going to help. The main idea here is to let ContentSigner close its resources by itself in its close() method. Then, allers are only responsible to close the ContentSigner and it then does all the required work by itself.

[dghgit]

Actually, I finally realised how this didn't work, although I can see why it might give the impression that it does.

The idea I guess is to do something like this:

  try (ContentSigner s = builder.build(key)) {
      OutputStream out = s.getOutputStream();
      out.write(data);
      byte[] sig = s.getSignature(); 
  }                                    

The problem is out.close() which should be called before getSignature(), ends up being called after getSignature(), which is too late. The convention in the APIs the calling out.close() is the equivalent of doFinal(), skipping that will mostly work for signatures at the moment (we probably should have enforced it...), but it's possible with some of the newer algorithms which require processing the data twice we may find ourselves in the situation where we have to take advantage of the signal from close().

Closing this as an interesting idea, but possibly a dangerous one.