Re: Automatic cleanup of lingering ResourceResolver's

[Konrad Windszus]

I created https://issues.apache.org/jira/browse/SLING-6375 for logging a WARN.

> On 8 Dec 2016, at 07:59, Konrad Windszus  wrote:
> 
> You are probably right that this is not that easy to detect and basically 
> there are always cases which you would not be able to detect.
> 
> Therefore I propose to at least log a big WARN in case a resolver has been 
> closed by the finalizer thread in 
> https://github.com/apache/sling/blob/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java#L100.
> This is in line with what Jackrabbit 2 did in 
> https://github.com/apache/jackrabbit/blob/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java#L1372.
> 
> IMHO not closing a resource resolver is incorrect usage of it and should 
> therefore not just silently lead to Sling closing those dangling resource 
> resolvers.
> WDYT?
> Konrad
> 
>> On 7 Dec 2016, at 10:12, Julian Sedding  wrote:
>> 
>> Hi Konrad
>> 
>> The only way I can imagine this to be implemented is by fully(!)
>> wrapping the JCR API and maintain a reference to originating
>> ResourceResolver in each object (e.g. Session, Node etc).
>> 
>> IMHO this is not worthwhile, as it opens a large can of worms. Imagine
>> you want to cast Session to JackrabbitSession, because you need access
>> to some Jackrabbit APIs. Would that be a supported use-case or not?
>> 
>> We could possibly delay closing the underlying Session until it is no
>> longer used, by a construct of WeakReferences and a ReferenceQueue.
>> But I don't think we could throw or log anything based on this,
>> because we cannot determine if the Session was still used or whether
>> the GC took a while to enqueue the reference (there are no guarantees
>> as to when a Reference is enqueued after it becomes eligible).
>> 
>> If there is a (simple and cheap) way to get a list of all referents of
>> a Session object that I am not aware of, then we could consider doing
>> something. Otherwise, we should make sure the API contract is explicit
>> about the fact that the underlying session is closed together with the
>> resource resolver (unless the RR was created based on an externally
>> created session), and leave it within a developer's responsibility to
>> handle correctly.
>> 
>> Regards
>> Julian
>> 
>> On Tue, Dec 6, 2016 at 6:06 PM, Konrad Windszus
>>  wrote:
>>> In https://issues.apache.org/jira/browse/SLING-4372 a mechanism was 
>>> invented which automatically closes all those resource resolvers which are 
>>> no longer referenced. That may lead to hard to detect issues in case 
>>> someone still uses a reference towards the underlying session (in case you 
>>> deal with a ResourceResolver acting on a JCR) or another object which was 
>>> adapted from that resource resolver. Would it be possible to somehow detect 
>>> open references to the underlying sessions or any other objects being 
>>> adapted from the resource resolver and issue a big WARN or ERROR in that 
>>> case? I know that this is a programming mistake but not that easy to spot 
>>> in some cases. Therefore additional logging in case such a situation is 
>>> detected would be very much appreciated.
>>> That would probably mean that we collect all references being issued 
>>> through the adaptTo method as well.
>>> WDYT? Is it worth the overhead?
>>> 
>>> Konrad
> 

Re: Automatic cleanup of lingering ResourceResolver's

[Konrad Windszus]

You are probably right that this is not that easy to detect and basically there 
are always cases which you would not be able to detect.

Therefore I propose to at least log a big WARN in case a resolver has been 
closed by the finalizer thread in 
https://github.com/apache/sling/blob/trunk/bundles/resourceresolver/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java#L100.
This is in line with what Jackrabbit 2 did in 
https://github.com/apache/jackrabbit/blob/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java#L1372.

IMHO not closing a resource resolver is incorrect usage of it and should 
therefore not just silently lead to Sling closing those dangling resource 
resolvers.
WDYT?
Konrad

> On 7 Dec 2016, at 10:12, Julian Sedding  wrote:
> 
> Hi Konrad
> 
> The only way I can imagine this to be implemented is by fully(!)
> wrapping the JCR API and maintain a reference to originating
> ResourceResolver in each object (e.g. Session, Node etc).
> 
> IMHO this is not worthwhile, as it opens a large can of worms. Imagine
> you want to cast Session to JackrabbitSession, because you need access
> to some Jackrabbit APIs. Would that be a supported use-case or not?
> 
> We could possibly delay closing the underlying Session until it is no
> longer used, by a construct of WeakReferences and a ReferenceQueue.
> But I don't think we could throw or log anything based on this,
> because we cannot determine if the Session was still used or whether
> the GC took a while to enqueue the reference (there are no guarantees
> as to when a Reference is enqueued after it becomes eligible).
> 
> If there is a (simple and cheap) way to get a list of all referents of
> a Session object that I am not aware of, then we could consider doing
> something. Otherwise, we should make sure the API contract is explicit
> about the fact that the underlying session is closed together with the
> resource resolver (unless the RR was created based on an externally
> created session), and leave it within a developer's responsibility to
> handle correctly.
> 
> Regards
> Julian
> 
> On Tue, Dec 6, 2016 at 6:06 PM, Konrad Windszus
>  wrote:
>> In https://issues.apache.org/jira/browse/SLING-4372 a mechanism was invented 
>> which automatically closes all those resource resolvers which are no longer 
>> referenced. That may lead to hard to detect issues in case someone still 
>> uses a reference towards the underlying session (in case you deal with a 
>> ResourceResolver acting on a JCR) or another object which was adapted from 
>> that resource resolver. Would it be possible to somehow detect open 
>> references to the underlying sessions or any other objects being adapted 
>> from the resource resolver and issue a big WARN or ERROR in that case? I 
>> know that this is a programming mistake but not that easy to spot in some 
>> cases. Therefore additional logging in case such a situation is detected 
>> would be very much appreciated.
>> That would probably mean that we collect all references being issued through 
>> the adaptTo method as well.
>> WDYT? Is it worth the overhead?
>> 
>> Konrad

Re: Automatic cleanup of lingering ResourceResolver's

[Julian Sedding]

Hi Konrad

The only way I can imagine this to be implemented is by fully(!)
wrapping the JCR API and maintain a reference to originating
ResourceResolver in each object (e.g. Session, Node etc).

IMHO this is not worthwhile, as it opens a large can of worms. Imagine
you want to cast Session to JackrabbitSession, because you need access
to some Jackrabbit APIs. Would that be a supported use-case or not?

We could possibly delay closing the underlying Session until it is no
longer used, by a construct of WeakReferences and a ReferenceQueue.
But I don't think we could throw or log anything based on this,
because we cannot determine if the Session was still used or whether
the GC took a while to enqueue the reference (there are no guarantees
as to when a Reference is enqueued after it becomes eligible).

If there is a (simple and cheap) way to get a list of all referents of
a Session object that I am not aware of, then we could consider doing
something. Otherwise, we should make sure the API contract is explicit
about the fact that the underlying session is closed together with the
resource resolver (unless the RR was created based on an externally
created session), and leave it within a developer's responsibility to
handle correctly.

Regards
Julian

On Tue, Dec 6, 2016 at 6:06 PM, Konrad Windszus
 wrote:
> In https://issues.apache.org/jira/browse/SLING-4372 a mechanism was invented 
> which automatically closes all those resource resolvers which are no longer 
> referenced. That may lead to hard to detect issues in case someone still uses 
> a reference towards the underlying session (in case you deal with a 
> ResourceResolver acting on a JCR) or another object which was adapted from 
> that resource resolver. Would it be possible to somehow detect open 
> references to the underlying sessions or any other objects being adapted from 
> the resource resolver and issue a big WARN or ERROR in that case? I know that 
> this is a programming mistake but not that easy to spot in some cases. 
> Therefore additional logging in case such a situation is detected would be 
> very much appreciated.
> That would probably mean that we collect all references being issued through 
> the adaptTo method as well.
> WDYT? Is it worth the overhead?
>
> Konrad

Automatic cleanup of lingering ResourceResolver's

[Konrad Windszus]

In https://issues.apache.org/jira/browse/SLING-4372 a mechanism was invented 
which automatically closes all those resource resolvers which are no longer 
referenced. That may lead to hard to detect issues in case someone still uses a 
reference towards the underlying session (in case you deal with a 
ResourceResolver acting on a JCR) or another object which was adapted from that 
resource resolver. Would it be possible to somehow detect open references to 
the underlying sessions or any other objects being adapted from the resource 
resolver and issue a big WARN or ERROR in that case? I know that this is a 
programming mistake but not that easy to spot in some cases. Therefore 
additional logging in case such a situation is detected would be very much 
appreciated.
That would probably mean that we collect all references being issued through 
the adaptTo method as well.
WDYT? Is it worth the overhead?

Konrad