That looks very promising. Is the map returned by that method always intended to be mutable? If so, that probably should be mentioned in the Javadoc, so it can be more formally, if not contractually, defined. Based on [1], it looks like it would be mutable.
[1]: https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/master/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java#L1170-L1175 //Paul On Thu, Jul 27, 2023 at 11:56 PM Carsten Ziegeler <[email protected]> wrote: > 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] >
