Ensure Host connections closed immediately upon ExtendedHostResolver refresh #237
Conversation
| Many thanks for contributing. We are obliged by law to have a signed scanned copy of a Contributor License Agreement (CLA) to be able to accept pull requests to the official ArangoDB repository - even for such small changes. We use an Apache 2 CLA for ArangoDB and its companion projects, which can be found here: https://www.arangodb.com/documents/cla.pdf Thank you |
| try { | ||
| c.close(); | ||
| } catch (Exception e) { | ||
| // TODO: at least log a message. can we use SLF4J? |
There was a problem hiding this comment.
Good idea. Yes, slf4j is a valid option here.
| } | ||
| | ||
| @Override | ||
| public void close() throws IOException { |
There was a problem hiding this comment.
You don't throw IOException now so no need in throws here.
| @Override | ||
| protected void finalize() throws Throwable { | ||
| super.finalize(); | ||
| close(); |
There was a problem hiding this comment.
There is no guarantee finalize method will be invoked ever in Java. So having this update makes little sense. Moreover relying on cleaning resources here is dangerous, since resources will be not disposed on time.
Instead we can make this class implementing Closeable interface so that its users will know that they should close it manually.
| Closing as no longer relevant. |
5 participants
When ExtendedHostResolver.resolve() is called and .isExpired() evaluates to true, we need to explicitly close each Host instance in the hosts list before clearing out the list. This is to ensure the connection pools managed in each Host (i.e. HostImpl) instance get cleaned up and prevent a memory leak.
This also most likely fixes #233 .