accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <>
Subject Re: Resource leak warnings
Date Fri, 13 Dec 2013 21:34:24 GMT
On Fri, Dec 13, 2013 at 4:02 PM, Mike Drob <> wrote:
>> If you're using Eclipse, did you know that you can delete warnings?

They show up again after each build. It's too much noise.

>> Given that by making the interface "Closeable", we're in effect
>> recommending that users close it, we should probably follow our own
>> recommendation, so "2" is probably not a good idea, and "3" is
>> probably better. I don't have time to go back and do "3", though.
> What is your time frame for 3 happening? I obviously can't promise anything
> for today, but can attempt to address this next week.

I'm satisfied with just #4, so I'm not pushing for #3. If somebody
contributes a patch to do it before 1.6 release (or maybe 1.4.5 if
that gets released first), that's okay with me.

>> "4" might be a good option, at least for 1.4.5-SNAPSHOT and
>> 1.5.1-SNAPSHOT, so we don't convey the idea (which represents a change
>> in API semantics) that you *should* close the Instance. Rather, it
>> conveys the idea that it's optional, which I think is more consistent
>> with those previous versions, and is suitable for the vast majority of
>> use cases.
> Making things Closeable was an intermediate step to making them
> AutoCloseable later. As you said in chat, these should have always been
> Closeable, but we just never gave clients a way to do that. It caused tons
> of heartache for users running from inside of web containers, and I think
> the existence of the interface is a strong hint for best practices.

Which is why I think #4 was a good compromise... because the close
method would still be there to serve that purpose, but it wouldn't
convey users *must* close it (thereby making their code incompatible
with 1.4.0-1.4.4 and 1.5.0), and it solves my immediate need of having
a clean build.

>> All of this is completely overshadowing the real issue, though, which
>> is that the close method doesn't actually prevent the resources from
>> being opened again. It's a superficial fix, that doesn't really
>> enforce it. Our API looks like it's stateless, with factory methods...
>> but it's not actually stateless. We can close the instance, but the
>> resources that were left open aren't isolated to the instance... they
>> are used inside the Connector and below. Closing the instance may free
>> up resources, but it doesn't stop new ones from being opened again
>> inside the connector and below. The problem is that the "Instance"
>> object does not fully represent the resources used inside client code,
>> so closing it is semantically unintuitive, incorrect, and functionally
>> broken if not used in a very specific way.
> Can you elaborate on this? Closing an instance should close all of the
> underlying objects as well (and I was under the impression that it did).

None of the underlying objects are Closeable (either with that
interface, or even semantically). Take Connector, for example. How do
you define the semantics for "closing" a factory? It's not
impossible... but it's a bit unintuitive... and very different
semantics from prior releases.

Christopher L Tubbs II

View raw message