Well, that depends - it is a complicated and expensive clean up if you
rely on the JVM; the request listener approach provides a valuable hook
to have a simple and near real time clean up - which usually is better
than waiting for the GC to step in.
But I'm wondering if we could simplify the whole thing by tying the life
cycle of a model to the resource resolver? With [1] we have a way to add
a Closeable to the resource resolver which is called when the resolver
is closed. If we could use that mechanism, then we can combine both
wishes in one solution: near time clean up but not bound to a request.
[1]
https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L883
Regards
Carsten
On 27.07.2023 21:10, Paul Bjorkstrand wrote:
If anything, I would question the use of the servlet request life cycle at
all. When a request is *not* the adaptable, then the cleanup is based on
the JVM lifecycle of the adapted model object instead. Would there be any
harm in doing the same for all model/adaptable combinations, and removing
the special handling for servlet request adaptables?
//Paul
On Thu, Jul 27, 2023 at 2:29 AM Carsten Ziegeler <[email protected]>
wrote:
What is the use case of having a fake request object, adapting it to a
model and not processing it through the Sling engine?
Regards
Carsten
On 27.07.2023 09:15, Dirk Rudolph wrote:
How would this avoid the queue?
Theoretically I can have a
o.a.s.api.request.builder.impl.SlingHttpServletRequestImpl instance
and adapt it to a model class without going through the Sling Engine.
We need the queue also for adaptables that are not necessarily bound
to the request lifecycle (e.g. Resource).
Regards,
Dirk
On Thu, 27 Jul 2023 at 06:50, Carsten Ziegeler <[email protected]>
wrote:
Hi,
I think at some point the servlet engine was actually reusing request
objects and we ran into trouble with an async cleanup based on request
objects and their attributes. Switching to the listener solved the
issue.
I'm not sure whether this is still the same or not. But I think getting
the listener support into the Engine is not that complicated and it
avoids the queue.
Regards
Carsten
On 26.07.2023 21:17, Paul Bjorkstrand wrote:
That was very helpful, thank you. Since requests would be (eventually)
garbage collected by the JVM, I feel that a reference/queue-only
approach
could be sufficient for all types of cleanup. Sure, the servlet
listener
method could improve the timing of when the cleanup executes, but I
imagine
it is not necessary (unless something, somewhere in the servlet
framework
plumbing is caching request objects and reusing them...which I hope
not).
I appreciate the insights!
//Paul
On Wed, Jul 26, 2023 at 10:50 AM Carsten Ziegeler <
[email protected]>
wrote:
A while back we had a similar discussion. Now I think this distinction
between real and fake requests could be completely removed,
simplifying
the models impl a lot - if we enhance the contract for servlet
listeners
being also called by Sling Engine for internal requests (which often
use
those fake request objects).
Regards
Carsten
On 26.07.2023 17:29, Dirk Rudolph wrote:
Hey Paul,
afaik the marker attribute it used to distinguish between real and
synthetic request objects.
The ServletRequestListner implementation of the ModelAdapterFactory
is
only called for requests created by the servlet container.
There are use cases that do not have access to a request object and
create a synthetic instance to adapt it to a Sling Model (directly or
indirectly), for example using the SlingHttpServletRequestBuilder
[1].
In this case requestDestroyed is never called with that request
object
and hence the cleanup mechanism needs to fall back to the one used
for
non-Request adaptables.
Best regards,
Dirk
[1]:
https://sling.apache.org/apidocs/sling12/org/apache/sling/api/request/builder/SlingHttpServletRequestBuilder.html
On Wed, 19 Jul 2023 at 22:11, Paul Bjorkstrand
<[email protected]> wrote:
I'm looking into a potential refactor of the Sling Models Impl to
make
the
code easier to manage, but also with an eye to enhancing the
functionality
in some ways that are hard to do with the way the code is currently
structured Examples of such improvements: support other "container
types"
in a generic fashion (Optional, WeakReference, and all
Collections/arrays)
and implement a java.lang.invoke.MethodHandle based injection
system.
One
area I have almost no knowledge about is the cleanup side with the
disposal
registry, and its related functionality.
As I have been reading through the code, I immediately noticed that
there
are two paths that are taken, depending on a couple factors, most
notably
whether the adaptable is a request or not. At first glance, it seems
that
this decision is a necessary difference between request adapted
models
or
resource adapted models. Digging in deeper, I believe this is not
the
case,
and it is just a performance/reliability improvement. This makes it
not
necessary to rely on PhantomReference for request adapted models,
since
there is a servlet request listener that notifies creation and
cleanup
of
the requests. This seems to be confirmed after reading [1].
What I don't quite understand is the purpose of
REQUEST_MARKER_ATTRIBUTE
[2] [3] [4] [5]. What does that do, and why is it needed? Any help
understanding this is appreciated!
[1]: https://issues.apache.org/jira/browse/SLING-5668
[2]:
https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L106C33-L106C58
[3]:
https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L688
[4]:
https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L774
[5]:
https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L1392
// Paul
--
Carsten Ziegeler
Adobe
[email protected]
--
Carsten Ziegeler
Adobe
[email protected]
--
Carsten Ziegeler
Adobe
[email protected]
--
Carsten Ziegeler
Adobe
[email protected]
