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

AblyRealtime should implement Autocloseable #514

Closed
amihaiemil opened this issue Nov 2, 2019 · 4 comments · Fixed by #515
Closed

AblyRealtime should implement Autocloseable #514

amihaiemil opened this issue Nov 2, 2019 · 4 comments · Fixed by #515

Comments

@amihaiemil
Copy link
Contributor

amihaiemil commented Nov 2, 2019

I noticed all or most of our tests for AblyRealtime have the try-catch-finally pattern, becuase the instantiated AblyRealtime objects need to be closed. I suspect that also in "real" usage, the object has to be closed when its no longer needed.

In order to simplify things, this class should implement Autocloseable and be used in try-with-resources, which was introduced in Java 7. Then, the code can be simplified a lot for our clients and also in our tests. Instead of this:

try {
    AblyRealtime ably = ...
} catch (AblyException ex) {
    fail(...);
} finally {
    ably.close();
}

we will have this:

try (AblyRealtime ably = ...) {
    ...
} // we don't care about the exception anymore
//since the test will fail automatically and log
//the exception anyway, if it is thrown.

Note that this change will not affect backwards compatibility!

@paddybyers
Copy link
Member

Is that going to be possible, given that we already declare public void close() ?

@amihaiemil
Copy link
Contributor Author

@paddybyers yes, it will be possible: implementors of an interface can choose to implement more specific methods: this means AblyRealtime.close() is fine as it is, we don't need to declare any throws Exception :)

It's also stated in Autocloseable's javadoc:

While this interface method is declared to throw Exception,
implementers are strongly encouraged to declare concrete implementations
of the close method to throw more specific exceptions,
or to throw no exception at all if the close operation cannot fail.

@paddybyers
Copy link
Member

Right, but it was the public bit I wasn't sure about.

@amihaiemil
Copy link
Contributor Author

@paddybyers yes, public is fine, it's how it should be :)

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

Successfully merging a pull request may close this issue.

2 participants