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]
