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

Closeable ContentSigner #822

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artem-smotrakov
Copy link
Contributor

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.

- Updated interface ContentSigner to extend AutoCloseable
- Implemented close() method in several anonymous classes
  that implement ContentSigner
@dghgit
Copy link
Contributor

dghgit commented Nov 20, 2020

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

@artem-smotrakov
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants