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 <cziege...@apache.org> 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 <cziege...@apache.org> > 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 < > cziege...@apache.org> > >>> 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 > >>>>> <paul.bjorkstr...@gmail.com> 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 > >>>> cziege...@apache.org > >>>> > >>> > >> > >> -- > >> Carsten Ziegeler > >> Adobe > >> cziege...@apache.org > > -- > Carsten Ziegeler > Adobe > cziege...@apache.org >
