accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
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 <mdrob@cloudera.com> wrote:
[snip]
>> 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).
[snip]

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
http://gravatar.com/ctubbsii

Mime
View raw message