Re: FYI, playing with Sling, GraalVM and native images

[Paul Bjorkstrand]

>From what I know of GraalVM/native-image, the biggest hurdles will be with
classloading (lazy load/hiding bundle-private classes) and reflection. The
nature of how Felix/OSGi hides nonpublic classes (like you stated #4 on
SLING-8556) would make it extremely difficult to get this working without
major changes.

Reflection can be worked around, by providing a list of what can be found
by reflection, so native-image can make that statically available to the
stripped down application.

Removing reflection and dynamic class loading means it would be impossible
(or nearly so) to make running application bundle changes. At that point,
you might as well be not using OSGi at all.

The idea of PojoSR is interesting, though, and (as Robert said) would be a
good starting point. It is likely that the entirety of Sling would need to
be able to run without being inside an OSGi container, before you can get
native-image to work properly. That effort, while probably huge, may be
easier than getting Felix to work with native-image.

Paul Bjorkstrand


On Wed, Jul 3, 2019 at 6:08 AM Robert Munteanu  wrote:

> On Wed, 2019-07-03 at 13:00 +0200, Bertrand Delacretaz wrote:
> > Hi Robert,
> >
> > On Wed, Jul 3, 2019 at 12:25 PM Robert Munteanu 
> > wrote:
> > > ...Have you considered using FelixConnect [1]
> > > instead of the OSGi mocks?...
> >
> > Yes that's one of the options as well.
> >
> > I haven't found docs or tests for that module however so I'm a bit
> > wary of using it, but I'll keep it in the back of my mind!
>
> Oak has a module using Felix Connect - back when it was called PojoSR.
>
>   https://github.com/apache/jackrabbit-oak/tree/trunk/oak-pojosr
>
> If you decide to try it out it can be a good starting point.
>
> Thanks,
>
> Robert
>
>

Re: [DISCUSS] Automatic Mapping of URIs in HTL

[Paul Bjorkstrand]

I like your idea of hooking into the existing URI context processing,
especially because you would want to have the mapping occur after URL
manipulation (I think). My thoughts based on what has been said above:

1. Hook into the existing URI context, and implement the processing
wherever URIs are processed (either automatic via context sensitive
handling built into HTL, or manual whenever context='uri' ise set for an
expression).
2. Have a configuration (CAConfig / OSGi / whatever) to drive whether to
map URIs or not for a given path and/or (super) resource type.
3. Have a local configuration to override the default that is set in the
previous value, like your mapUri, but with a boolean value to turn it
on/off (mapUri=true / mapUri=false)

The reason I think both the global config and the local config are
necessary: JSON or some JS objects in HTL scripts (thinking on how many
times I had to go back and change a Sling Model to add URI mapping, because
a specific Sling-based product that uses a servlet filter does not/cannot
map values in JS/JSON).

-Paul


On Fri, Sep 11, 2020 at 8:15 AM Georg Henzler  wrote:

> Hi Radu,
>
>
> >> 1. Per URI as mentioned in template:
> >>  
> >
> > This could work - it would be a Sling-specific option and we’ve done
> > this in the past in https://issues.apache.org/jira/browse/SLING-5812
> > . The option would
> > introduce a new runtime call to the new SPIs.
>
> I quickly tested this with minor changes to
> URIManipulationFilterExtension
> and URIManipulationFilter and it worked fine - but as said it's
> tedious and error prone to go this route, it should rather be a cross
> cutting concern
>
> >> 3. Per global OSGi configuration
> >>
> > How would we handle 3?
>
> So one simple naive way would be to hook into [1], check for the
> "uri" context and call the map API method on the value. The probably
> clean way (I don't know this code very well) is to create a dedicated
> filter that gets to see all properties similar to the XSSFilter (and
> adjusts uri values only).
>
> I also think conceptually it also fits in well with the "automatic
> context sensitivity" in the spec [2] (just another aspect compared to
> XSS). While the template/spec does not change, the implementation
> takes care of the handling and configuration, this is true for XSS
> today and would also hold true for mapping of URIs.
>
> -Georg
>
>
> [1]
>
> https://github.com/apache/sling-org-apache-sling-scripting-sightly-compiler/blob/eb8891e89caef2993f4cdab0fbe764a7695ec0e8/src/main/java/org/apache/sling/scripting/sightly/impl/filter/XSSFilter.java#L51
>
> [2]
>
> https://github.com/adobe/htl-spec/blob/1.2/SPECIFICATION.md#113-context-sensitive
>

Re: [models] declaring constants for injectors

[Paul Bjorkstrand]

If the goal is to dissuade the use of the @Source, it might be worthwhile
to deprecate the annotation, and come up with a new one that is only
allowed on ElementType.ANNOTATION_TYPE. The deprecation, along with the
mentioned documentation changes, would signal that @Source should not be
used, and that an injector-specific annotation should be used (or created,
in the case of custom injectors) instead.

-Paul


On Tue, Sep 1, 2020 at 3:46 AM Nicolas Peltier 
wrote:

> so should we remove the "Available Injectors" entirely? move it after
> "Injector-specific annotation", or more clearly specify you should not use
> @source but those annotations?
> i'm in favour of the first one. Looks like there is quite a lot of legacy
> ways we should move elsewhere or remove
>
> Le lun. 31 août 2020 à 23:10, Stefan Seifert  a
> écrit :
>
> > you're right - the "@Inject @Source" pattern is the "old-style" syntax
> > which we still have to support for backwards compatibility but probably
> > should discourage in the docs and position the injector-specific
> > annotations more prominently, as they are the recommended way in my pov.
> >
> > stefan
> >
> > >-Original Message-
> > >From: Nicolas Peltier 
> > >Sent: Monday, August 31, 2020 1:22 PM
> > >To: Sling Developers List 
> > >Subject: Re: [models] declaring constants for injectors
> > >
> > >i guess doc is a bit misleading here and we should not expose what you
> > >righteously call "implementation details" in the documentation (see
> > >"script-bindings" in
> > >https://sling.apache.org/documentation/bundles/models.html)
> > >
> > >Le lun. 31 août 2020 à 12:34, Nicolas Peltier <
> peltier.nico...@gmail.com>
> > a
> > >écrit :
> > >
> > >> mm i thought some of them were only available through the @Source
> > >> annotation, but i will work my models annotations a bit more :-)
> > >>
> > >> Le lun. 31 août 2020 à 12:10, Stefan Seifert 
> a
> > >> écrit :
> > >>
> > >>> these string constants are normally not used in application code,
> > >because
> > >>> they use the typed injector annotations.
> > >>> so this is more an implementation detail and there is no need to
> > publish
> > >>> those constants as part of the API?
> > >>>
> > >>> stefan
> > >>>
> > >>> >-Original Message-
> > >>> >From: Nicolas Peltier 
> > >>> >Sent: Monday, August 31, 2020 12:07 PM
> > >>> >To: Sling Developers List 
> > >>> >Subject: [models] declaring constants for injectors
> > >>> >
> > >>> >Hey
> > >>> >
> > >>> >all injectors have name declared as direct string constants in
> getName
> > >>> >implementations (script-bindings, child-resources, self, ...)
> > >>> >not entirely sure it's still best practice to use constants rather
> > than
> > >>> >literals in annotation, but if it's still the case, i guess
> declaring
> > &
> > >>> >exposing those literals in the API would make sense.
> > >>> >
> > >>> >Wdyt?
> > >>> >
> > >>> >Nicolas
> > >>>
> > >>
> >
>

Re: [DISCUSS] New Content API/Modules

[Paul Bjorkstrand]

Glad to see that :-) I am a big fan of the promise-like feel of
CompletableFuture (at least until Project Loom [1] is complete).

-Paul
[1]: https://openjdk.java.net/projects/loom/

On Tue, Aug 11, 2020 at 1:35 PM Oliver Lietz  wrote:

> On Tuesday, August 11, 2020 6:42:32 PM CEST Paul Bjorkstrand wrote:
> > If I may suggest, use CompletableFuture instead of Future. IMO it is a
> more
> > natural API, and helps to steer people away from using the blocking
> method
> > Future.get(). It's hard enough to ensure features are developed properly
> > for a multithreaded application without adding in thread-blocking "async"
> > operations. (This assumes you were intending to use Future as a means of
> > denoting an asynchronous operation.)
> >
> > A side benefit: you may be able to get away with not building a custom
> > pipeline API, and just make methods that return a CompletionStage. The
> > benefits of this are questionable, but it may help with prototyping or
> > implementing under the covers.
>
> Thanks, Paul! Completable just got lost somewhere. I had CompletableFuture
> in
> my mind as I'm quite happy with what we have in Commons Messaging Mail:
>
>
> https://github.com/apache/sling-org-apache-sling-commons-messaging-mail/blob/
>
> master/src/main/java/org/apache/sling/commons/messaging/mail/MailService.java
> <https://github.com/apache/sling-org-apache-sling-commons-messaging-mail/blob/master/src/main/java/org/apache/sling/commons/messaging/mail/MailService.java>
>
> Regards,
> O.
>
>
> > -Paul
> >
> >
> > On Tue, Aug 11, 2020 at 8:07 AM Bertrand Delacretaz <
> bdelacre...@apache.org>
> > wrote:
> > > Hi,
> > >
> > > On Tue, Aug 11, 2020 at 2:11 PM Oliver Lietz 
> > >
> > > wrote:
> > > > ...I would like to discuss new API and modules for content analyzing
> and
> > > > processing...
> > >
> > > I think it's often useful to be able to chain such things in a similar
> > > way to Unix Pipes.
> > >
> > > Which might speak for an API like
> > >
> > > FilePipeStatus s = FilePipe.
> > >
> > >   .withProcessor(grep)
> > >   .withProcessor(sed)
> > >   .withProcessor(antivirus)
> > >   .withInput(someInputStream)
> > >   .withOutput(someOutputStream)
> > >   .start()
> > >
> > > WhereFilePipeStatus provides access to the completion Future and to
> > > the processing report, which might also be modeled along the lines of
> > > standard output / error.
> > >
> > > Just my 2 cents - but I think a pipeline model can be useful here (and
> > > we already have something named Sling Pipes)
> > >
> > > -Bertrand
>
>
>
>
>

Re: [DISCUSS] New Content API/Modules

[Paul Bjorkstrand]

If I may suggest, use CompletableFuture instead of Future. IMO it is a more
natural API, and helps to steer people away from using the blocking method
Future.get(). It's hard enough to ensure features are developed properly
for a multithreaded application without adding in thread-blocking "async"
operations. (This assumes you were intending to use Future as a means of
denoting an asynchronous operation.)

A side benefit: you may be able to get away with not building a custom
pipeline API, and just make methods that return a CompletionStage. The
benefits of this are questionable, but it may help with prototyping or
implementing under the covers.

-Paul


On Tue, Aug 11, 2020 at 8:07 AM Bertrand Delacretaz 
wrote:

> Hi,
>
> On Tue, Aug 11, 2020 at 2:11 PM Oliver Lietz 
> wrote:
> > ...I would like to discuss new API and modules for content analyzing and
> > processing...
>
> I think it's often useful to be able to chain such things in a similar
> way to Unix Pipes.
>
> Which might speak for an API like
>
> FilePipeStatus s = FilePipe.
>   .withProcessor(grep)
>   .withProcessor(sed)
>   .withProcessor(antivirus)
>   .withInput(someInputStream)
>   .withOutput(someOutputStream)
>   .start()
>
> WhereFilePipeStatus provides access to the completion Future and to
> the processing report, which might also be modeled along the lines of
> standard output / error.
>
> Just my 2 cents - but I think a pipeline model can be useful here (and
> we already have something named Sling Pipes)
>
> -Bertrand
>

Re: Cache objects retrieved/derived from a ResourceResolver

[Paul Bjorkstrand]

As far as I understand, thread locals' storage is an implementation detail
in the JVM. There isn't an API to clear all/arbitrary thread locals. Thread
pools, or applications that use thread pools, need to provide hooks to do
that, so that code can do its own cleanup. This is exactly what the
request listener that Carsten mentioned does.

Also, thread pools keep threads for long periods of time (up to the entire
run of the application). There is no guarantee that a thread will be GC'd
ever in the lifetime of an application. That is the point of thread pools
in the first place as threads are relatively expensive to create [1]...at
least expensive until Project Loom [2] is finished.

[1]: https://stackoverflow.com/a/5483105
[2]: https://openjdk.java.net/projects/loom/

-Paul


On Sun, Oct 17, 2021 at 10:53 AM Carsten Ziegeler 
wrote:

> Hi,
>
> not sure - this heavily depends on how the servlet engine manages the
> threads. I guess usually a thread pool is used, so threads are reused.
> I'm not sure whether thread locals are cleaned up when a thread is
> returned into the pool by every engine and if you can rely on it.
> But using a request listener and cleaning up the thread local works in
> all cases :)
>
> Carsten
>
> Am 17.10.2021 um 13:56 schrieb Roy Teeuwen:
> > Hey Carsten,
> >
> > Could you clarify why you need to clean up the threadlocal when it is
> bound to the thread of a request? Don't the threads of a request get
> garbage collected, and by that way also the ThreadLocal's bound to it
> >
> > Greets,
> > Roy
> >
> >> On 17 Oct 2021, at 09:57, Carsten Ziegeler 
> wrote:
> >>
> >> Thanks Jörg, initially I got your use case wrong.
> >>
> >> As several people have adviced here now, using an adapter factory
> should give you what you need.
> >>
> >> A more ugly version would be to use a thread local and clean it up when
> the request finishes (via a request listener)
> >>
> >> Another option would be to write a custom resource provider, but that
> might be even more a hack than anything else.
> >>
> >> Regards
> >> Carsten
> >>
> >> Am 16.10.2021 um 16:41 schrieb Jörg Hoh:
> >>> Hi Carsten,
> >>> Am Sa., 16. Okt. 2021 um 09:41 Uhr schrieb Carsten Ziegeler <
> >>> cziege...@apache.org>:
>  I don't think that the RR is the right place for this.
> 
>  If the use case is that during a request the exact same query is
>  executed more than once, then caching the result of the query in Sling
>  is probably not resulting in the wanted gain. I think it is much
> better
>  to avoid executing the query multiple times in the first place -
> 
> >>> That's what I want to achieve. But where should I store the result of
> the
> >>> query so I don't need to execute the query multiple times? As said, I
> don't
> >>> have access to a store, which allows me to store this result within a
> >>> well-defined scope (which is identical to the lifetime of a resource
> >>> resolver). And I don't want to roll such a logic in my own, because
> it's
> >>> definitely not easy to get that right.
>  executing the query is one part but then processing the result is
>  another. And you want to avoid this as well.
> 
> >>> In every invocation of my method a different resource is passed, and I
> need
> >>> to evaluate the result of that query for this resource. To be exact, I
> >>> transform the result of the query into other domain objects, and to
> >>> optimize the performance of this evaluation, I might even store these
> >>> domain objects in a different structure than just a list.
> 
>  In the past 12 years or so we tried various POCs with caching in the
> RR.
>  Always under the (wrong) assumption that Oak/Jackrabbit is the
>  bottleneck. So we added a RR caching all get resources, all list
>  resources, all queries etc. And it turned out that the gain was close
> to
>  zero. Each operation in Oak was pretty fast, but if you execute it
>  hundreds of times during a request, even a fast call becomes the
>  bottleneck. So again, usually the problem is in the upper layers,
> above
>  Sling's resource api.
> 
> >>> I don't want to create a transparent caching layer on a RR level to
> store
> >>> objects which have been requested from the repository. I know that
> this was
> >>> tried in the past, and did not provide the benefits we intended it to
> have.
> >>> I just want to have a simple store (map) on the RR, where I can store
> all
> >>> types of objects, which share the same lifecycle as the resource
> resolver.
> >>> This is handled explicitly, and the developer is responsible to make
> sure,
> >>> that indeed the lifecycles of the resource resolver and the object
> which
> >>> are stored in this temporary storage are compatible. If the developer
> >>> decides to store resources in there, and these resources might change
> over
> >>> the lifetime of this (potentially long-living) resource resolver, so
> be it.
> >>> There should not be any magic, it's 

Re: Cache objects retrieved/derived from a ResourceResolver

[Paul Bjorkstrand]

My point stands, there is no API provided. The thread local cleaner that is
used, while used commonly in many applications (I am 99% sure I saw that
exact code or very similar in a SO while looking for such an API), is still
dependent on internal implementation details of thread locals
(non-public/inaccessible fields). This is no different than depending on
Unsafe. As you mentioned, it is much harder in later versions of Java
because implementation details are much more encapsulated.

-Paul


On Mon, Oct 18, 2021 at 4:24 AM Robert Munteanu  wrote:

> Hi,
>
> On Sun, 2021-10-17 at 14:26 -0500, Paul Bjorkstrand wrote:
> > As far as I understand, thread locals' storage is an implementation
> > detail
> > in the JVM. There isn't an API to clear all/arbitrary thread locals.
> > Thread
> > pools, or applications that use thread pools, need to provide hooks to
> > do
> > that, so that code can do its own cleanup. This is exactly what the
> > request listener that Carsten mentioned does.
>
> Sling's thread pool actually cleans up thread locals after execution
> [1]. It's not yet fixed for Java 17 though [2].
>
> [1]:
>
> https://github.com/apache/sling-org-apache-sling-commons-threads/blob/org.apache.sling.commons.threads-3.2.22/src/main/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocals.java
> [2]: https://issues.apache.org/jira/browse/SLING-10831
>
> Thanks,
> Robert
>

Re: Cache objects retrieved/derived from a ResourceResolver

[Paul Bjorkstrand]

On Sat, Oct 16, 2021 at 9:42 AM Jörg Hoh 
wrote:

> Hi Carsten,
>
> Am Sa., 16. Okt. 2021 um 09:41 Uhr schrieb Carsten Ziegeler <
> cziege...@apache.org>:
>
> > I don't think that the RR is the right place for this.
> >
> > If the use case is that during a request the exact same query is
> > executed more than once, then caching the result of the query in Sling
> > is probably not resulting in the wanted gain. I think it is much better
> > to avoid executing the query multiple times in the first place -
> >
>
> That's what I want to achieve. But where should I store the result of the
> query so I don't need to execute the query multiple times? As said, I don't
> have access to a store, which allows me to store this result within a
> well-defined scope (which is identical to the lifetime of a resource
> resolver). And I don't want to roll such a logic in my own, because it's
> definitely not easy to get that right.
>
>
Is it safe to assume that there are no concurrency concerns? If there are
no concerns, a field in your adapter object (that you get from
rr.adaptTo()) can store the result of the query. You can do a null check,
and only perform the query if it is null. That is pretty standard logic in
a lot of non-concurrent applications:

 queryResult;
...

 getResult()() {
  if (queryResult == null) {
queryResult = // some expensive query
  }

  return queryResult;
}

 processForResource(Resource r) {
  QueryResult result= getResult();
  // do your logic with the query result & resource
  // possibly storing intermediate results in other fields (maybe protected
with similar null-checks)

  return // ... the result of your computation
}

For the lifetime & scope problem, you get the benefits of the adapter cache
as part of the adaptTo logic.

If concurrency is a concern, then you would need to do some sort of
synchronization to ensure only one thread is performing the query. Given
that this is per-request, it sounds like you will not need synchronization.


> > executing the query is one part but then processing the result is
> > another. And you want to avoid this as well.
> >
>
> In every invocation of my method a different resource is passed, and I need
> to evaluate the result of that query for this resource. To be exact, I
> transform the result of the query into other domain objects, and to
> optimize the performance of this evaluation, I might even store these
> domain objects in a different structure than just a list.
>
>
If concurrency is not a problem, then you can do the same type of "caching"
in the adapter object.


> >
> > In the past 12 years or so we tried various POCs with caching in the RR.
> > Always under the (wrong) assumption that Oak/Jackrabbit is the
> > bottleneck. So we added a RR caching all get resources, all list
> > resources, all queries etc. And it turned out that the gain was close to
> > zero. Each operation in Oak was pretty fast, but if you execute it
> > hundreds of times during a request, even a fast call becomes the
> > bottleneck. So again, usually the problem is in the upper layers, above
> > Sling's resource api.
> >
>
> I don't want to create a transparent caching layer on a RR level to store
> objects which have been requested from the repository. I know that this was
> tried in the past, and did not provide the benefits we intended it to have.
> I just want to have a simple store (map) on the RR, where I can store all
> types of objects, which share the same lifecycle as the resource resolver.
>
> This is handled explicitly, and the developer is responsible to make sure,
> that indeed the lifecycles of the resource resolver and the object which
> are stored in this temporary storage are compatible. If the developer
> decides to store resources in there, and these resources might change over
> the lifetime of this (potentially long-living) resource resolver, so be it.
> There should not be any magic, it's just a simple map.
>
> Jörg
>
> --
> Cheers,
> Jörg Hoh,
>
> https://cqdump.joerghoh.de
> Twitter: @joerghoh
>

-Paul

Re: Discuss removing synchronized blocks from injection points within sling-models-impl

[Paul Bjorkstrand]

It may be possible to do that, by doing a reflective check on that method
and calling that method reflectively (or via method handles, since it is
public). The code might be a bit complex, but I'll give it a go.

-Paul


On Mon, Dec 6, 2021 at 3:22 AM Robert Munteanu  wrote:

> Hi Paul,
>
> On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
> > I added in using MethodHandles in the tests, just to show the
> > difference.
> > When Sling stops supporting Java 8, we can move to MethodHandle which
> > has
> > significantly better performance characteristics. Unfortunately, we
> > can't
> > make that move until we are on at least Java 9, so that we can use
> > MethodHandles.privateLookupIn(..) [2].
>
> Thanks for the comprehensive tests. Would it be possible to try
> MethodHandles and fall back to reflection is they're not available?
>
> Thanks,
> Robert
>

Re: Discuss removing synchronized blocks from injection points within sling-models-impl

[Paul Bjorkstrand]

Agreed, Konrad. That was the way I was going to approach it. I am adding
the "remove sync" ticket now, and will add the MethodHandles ticket after I
have the PR for sync removal in, since it is far simpler.

-Paul


On Mon, Dec 6, 2021 at 9:07 AM Konrad Windszus  wrote:

> Maybe we can start with the simple code by removing synchronized and
> reset.
> Using MethodHandle should be treated separately (in a dedicated ticket and
> PR).
>
> Konrad
>
> > On 6. Dec 2021, at 16:03, Paul Bjorkstrand 
> wrote:
> >
> > It may be possible to do that, by doing a reflective check on that method
> > and calling that method reflectively (or via method handles, since it is
> > public). The code might be a bit complex, but I'll give it a go.
> >
> > -Paul
> >
> >
> > On Mon, Dec 6, 2021 at 3:22 AM Robert Munteanu 
> wrote:
> >
> >> Hi Paul,
> >>
> >> On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
> >>> I added in using MethodHandles in the tests, just to show the
> >>> difference.
> >>> When Sling stops supporting Java 8, we can move to MethodHandle which
> >>> has
> >>> significantly better performance characteristics. Unfortunately, we
> >>> can't
> >>> make that move until we are on at least Java 9, so that we can use
> >>> MethodHandles.privateLookupIn(..) [2].
> >>
> >> Thanks for the comprehensive tests. Would it be possible to try
> >> MethodHandles and fall back to reflection is they're not available?
> >>
> >> Thanks,
> >> Robert
> >>
>
>

Discuss removing synchronized blocks from injection points within sling-models-impl

[Paul Bjorkstrand]

In light of discussion on a recently merged PR [1], I raised the question
'do we need to "reset" the accessible flag for reflective injectables
(fields, methods, constructors)?'

The sole purpose of synchronized around the injection logic (including
constructor instantiation) seems to surround properly resetting the
accessible flag on the reflection element, and ensuring the element is
accessible during the injection.

It is likely worth the effort to determine if resetting that flag, and thus
the accompanying synchronized, is really needed. As a reference, I searched
Apache Felix for uses of setAccessible, as it also needs to be able to
inject into non-public elements [2]. After browsing a handful of the
non-test classes, I found none that do anything more than
setAccessible(true). This leads me to believe that we are doing extra work
in Sling Models that isn't really necessary.

Removing synchronized could have a dramatic, positive impact on
performance, especially for situations where there may be high contention
(e.g. heavy model/submodel reuse, sharing a common super class, etc). If
those synchronizations were removed, overall throughput will likely
increase.

Refs:
[1]:
https://github.com/apache/sling-org-apache-sling-models-impl/pull/11/files#r762409049
[2]: https://github.com/apache/felix-dev/search?q=setAccessible

-Paul

Re: Discuss removing synchronized blocks from injection points within sling-models-impl

[Paul Bjorkstrand]

I tried to create a JMH to show the difference between various injection
techniques. While I am familiar with JMH and how to write them, I make no
claims that this is perfect. Also, I did not do any kind of analysis, such
as looking into the hot methods assembly.

I made eight different benchmarks, covering the following:

1. Set field directly (control).
2. Set field via setter method (alternate control).
3. Set field via reflection using Field object, not synchronized.
4. Set field via reflection using Method object for setter method, not
synchronized.
5. Set field via reflection using Field object, synchronized.
6. Set field via reflection using Method object for setter method,
synchronized.
7. Set field via MethodHandle using a handle to the field setter.
8. Set field via MethodHandle using a handle to the setter method.

You can see the full results & code at [1]. I only kept a few of the best
(lowest error value) runs. One of the best runs I have copied below, but
they are best viewed in monospace font, like at [1].

These numbers are specific to my machine, so the important inference that
can be gained from these data is the relative, not absolute values.

Benchmark  (str)  Mode
 CntScoreError  Units
SynchronizedBenchmark.viaField   str  avgt
  157.158 ±  0.181  ns/op
SynchronizedBenchmark.viaSetterMethodstr  avgt
  157.802 ±  0.193  ns/op
SynchronizedBenchmark.viaMethodHandle_setterMethod   str  avgt
  15   17.766 ±  0.551  ns/op
SynchronizedBenchmark.viaMethodHandle_field  str  avgt
  15   17.939 ±  0.290  ns/op
SynchronizedBenchmark.viaReflection_fieldstr  avgt
  15  157.453 ±  2.678  ns/op
SynchronizedBenchmark.viaReflection_setterMethod str  avgt
  15  157.346 ±  2.049  ns/op
SynchronizedBenchmark.viaReflection_setterMethod_synchronizedstr  avgt
  15  542.966 ± 19.825  ns/op
SynchronizedBenchmark.viaReflection_field_synchronized   str  avgt
  15  771.381 ± 47.922  ns/op

I added in using MethodHandles in the tests, just to show the difference.
When Sling stops supporting Java 8, we can move to MethodHandle which has
significantly better performance characteristics. Unfortunately, we can't
make that move until we are on at least Java 9, so that we can use
MethodHandles.privateLookupIn(..) [2].

Regardless, you can see the significant difference between synchronized vs
non-synchronized: synchronizing is 3-8x slower than not, when under
contention with multiple threads.

[1]:
https://gist.github.com/paul-bjorkstrand/f3bb154665e7d2168b4656eb7b794496
[2]:
https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.html#privateLookupIn-java.lang.Class-java.lang.invoke.MethodHandles.Lookup-
-Paul


On Sat, Dec 4, 2021 at 9:48 AM Paul Bjorkstrand 
wrote:

> In light of discussion on a recently merged PR [1], I raised the question
> 'do we need to "reset" the accessible flag for reflective injectables
> (fields, methods, constructors)?'
>
> The sole purpose of synchronized around the injection logic (including
> constructor instantiation) seems to surround properly resetting the
> accessible flag on the reflection element, and ensuring the element is
> accessible during the injection.
>
> It is likely worth the effort to determine if resetting that flag, and
> thus the accompanying synchronized, is really needed. As a reference, I
> searched Apache Felix for uses of setAccessible, as it also needs to be
> able to inject into non-public elements [2]. After browsing a handful of
> the non-test classes, I found none that do anything more than
> setAccessible(true). This leads me to believe that we are doing extra work
> in Sling Models that isn't really necessary.
>
> Removing synchronized could have a dramatic, positive impact on
> performance, especially for situations where there may be high contention
> (e.g. heavy model/submodel reuse, sharing a common super class, etc). If
> those synchronizations were removed, overall throughput will likely
> increase.
>
> Refs:
> [1]:
> https://github.com/apache/sling-org-apache-sling-models-impl/pull/11/files#r762409049
> [2]: https://github.com/apache/felix-dev/search?q=setAccessible
>
> -Paul
>

Introducing Lombok to Sling?

[Paul Bjorkstrand]

I am not a huge fan of boilerplate code, and getters/setters are my biggest
pet peeve. I searched the dev archives and found nothing that referenced
Lombok. Is there a reason it has not yet been introduced that I am not
aware of? If not, what is everyone's opinion around using Lombok within
Sling? I looked at the license, and it seems to be near-identical to HPND
[1] and HPND is one of ASF's Category A licenses [2]. It uses/includes ASM
[3] which uses a 3-clause BSD license [4], also one of the Category A
licenses.

While I wouldn't recommend allowing _everything_ from Lombok to be used, I
think it would be fair to allow the use of a subset of the features. I
would recommend the following approach:

These stabe features of Lombok [5] can be used without restriction:
- @Getter/@Setter
- @ToString
- @EqualsAndHashCode
- @NoArgsConstructor/@RequiredArgsConstructor/@AllArgsConstructor
- @Data/@Value (at least until we can use records [6], introduced in JDK
14, finalized in JDK 16)
- @Builder
- @Slf4j

All other stable features would be evaluated on a per-use basis. All
experimental features would not be allowed to be used.

These seem, to me, to be reasonable guidelines, and align with the usages I
have seen in other places (including my own projects).

[1]: https://opensource.org/licenses/HPND
[2]: https://www.apache.org/legal/resolved.html#category-a
[3]: https://asm.ow2.io/index.html
[4]: https://opensource.org/licenses/BSD-3-Clause
[5]: https://projectlombok.org/features/all
[6]: https://docs.oracle.com/en/java/javase/14/language/records.html


-Paul

Re: Introducing Lombok to Sling?

[Paul Bjorkstrand]

Sorry for the delayed reply, I sent that and went on a LONG staycation
where I (near literally) unplugged from anything remotely related to work.
To answer Konrad's question first: I don't have any particular module in
mind, just a question that popped in my head. Don't take my verbosity for
passion, though. I just wanted to provide more context/a start to the
conversation, rather than a "y u no lombok".

I am a fan of lombok, but for some projects it just doesn't make sense, and
Sling may just be one - per Konrad's point that it doesn't have a lot of
"data" carrier classes with getters/setters.

I totally agree with records (I even mentioned it in footnote [6] in the
first email). I am extremely eager to use newer Java features (switch
expressions, records, text blocks, pattern matching... , to name a few),
but am unfortunately constrained by the realities of how the corporate
world works.

Thanks for your inputs!

-Paul


On Mon, Jan 3, 2022 at 5:24 AM Stefan Seifert
 wrote:

> i would also favor to *not* add Lombok to any sling modules.
>
> stefan
>
> >-----Original Message-
> >From: Paul Bjorkstrand 
> >Sent: Thursday, December 23, 2021 10:47 PM
> >To: dev@sling.apache.org
> >Subject: Introducing Lombok to Sling?
> >
> >I am not a huge fan of boilerplate code, and getters/setters are my
> biggest
> >pet peeve. I searched the dev archives and found nothing that referenced
> >Lombok. Is there a reason it has not yet been introduced that I am not
> >aware of? If not, what is everyone's opinion around using Lombok within
> >Sling? I looked at the license, and it seems to be near-identical to HPND
> >[1] and HPND is one of ASF's Category A licenses [2]. It uses/includes ASM
> >[3] which uses a 3-clause BSD license [4], also one of the Category A
> >licenses.
> >
> >While I wouldn't recommend allowing _everything_ from Lombok to be used, I
> >think it would be fair to allow the use of a subset of the features. I
> >would recommend the following approach:
> >
> >These stabe features of Lombok [5] can be used without restriction:
> >- @Getter/@Setter
> >- @ToString
> >- @EqualsAndHashCode
> >- @NoArgsConstructor/@RequiredArgsConstructor/@AllArgsConstructor
> >- @Data/@Value (at least until we can use records [6], introduced in JDK
> >14, finalized in JDK 16)
> >- @Builder
> >- @Slf4j
> >
> >All other stable features would be evaluated on a per-use basis. All
> >experimental features would not be allowed to be used.
> >
> >These seem, to me, to be reasonable guidelines, and align with the usages
> I
> >have seen in other places (including my own projects).
> >
> >[1]: https://opensource.org/licenses/HPND
> >[2]: https://www.apache.org/legal/resolved.html#category-a
> >[3]: https://asm.ow2.io/index.html
> >[4]: https://opensource.org/licenses/BSD-3-Clause
> >[5]: https://projectlombok.org/features/all
> >[6]: https://docs.oracle.com/en/java/javase/14/language/records.html
> >
> >
> >-Paul
>

Re: Question about org.apache.sling.models.spi.DisposalCallbackRegistry & related

[Paul Bjorkstrand]

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 
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
> >  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
>

Re: Question about org.apache.sling.models.spi.DisposalCallbackRegistry & related

[Paul Bjorkstrand]

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 
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 
> > 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 
> >> 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
> >>

Re: Question about org.apache.sling.models.spi.DisposalCallbackRegistry & related

[Paul Bjorkstrand]

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 
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 
> 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
> >>>>>  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
>

Re: Sling Model Exporter: Prevent serializing of a ResourceResolver

[Paul Bjorkstrand]

Could you do something like [1] and [2] and just write out JSON null or
some other value that makes sense? That would avoid trying to serialize the
ResourceResolver (and potentially Resource as well), at the expense of a
remaining null. There also may be other features of the StdSerializer [3]
that can be used to prevent the field from being written at all, but I am
not familiar enough with it to say if it is possible or not.

[1]:
https://github.com/adobe/aem-core-wcm-components/blob/main/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/jackson/PageModuleProvider.java

[2]:
https://github.com/adobe/aem-core-wcm-components/blob/main/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/jackson/PageSerializer.java

[3]:
https://fasterxml.github.io/jackson-databind/javadoc/2.8/com/fasterxml/jackson/databind/ser/std/StdSerializer.html

//Paul


On Tue, Jun 27, 2023 at 12:11 PM Radu Cotescu  wrote:

> Hi Jörg,
>
> I think you would have the same problem with that model given the other
> property, namely that list of resources.
>
> Unfortunately I don’t see an easy way out other than documenting this
> aspect and advising developers to avoid exposing implementation details.
> Whatever you’d change in the implementation to prevent this, it would be at
> least a behaviour change, potentially causing regressions similar to
> https://xkcd.com/1172/.
>
> Regards,
> Radu
>
> On Tue, 27 Jun 2023 at 18:35, Jörg Hoh 
> wrote:
>
> > HI Stefan,
> >
> > I agree, but:
> >
> > I don't have control over the code, which causes the ResourceResolver to
> be
> > serialized. Also I cannot enforce, that such code is NOT written and
> > deployed (there are too many ways to make it wrong). Also because it
> > currently works, I would have to counter the "but it worked yesterday,
> and
> > I did not change anything since then" argument, in case it fails with an
> > exception like above.
> >
> > So I have to find another way to prevent the ResourceResolver from being
> > serialized ...
> >
> > Jörg
> >
> >
> >
> > Am Di., 27. Juni 2023 um 13:38 Uhr schrieb Stefan Seifert
> > :
> >
> > > well, using lombok with such a result is a bad idea. there should not
> be
> > a
> > > getResolver() method, this is an implementation detail and no getter
> > should
> > > be provided for it.
> > >
> > > so to put it another way: good that the serialization fails, so you can
> > > fix the calls to not add a getResolver() method!
> > >
> > > stefan
> > >
> > > p.s. i'm not a fan of lombok in general and have no experience with it,
> > > but I assume it can be configured to not expose the resolver.
> > >
> > > > -Original Message-
> > > > From: Jörg Hoh 
> > > > Sent: Tuesday, June 27, 2023 1:28 PM
> > > > To: Sling Developers List 
> > > > Subject: Sling Model Exporter: Prevent serializing of a
> > ResourceResolver
> > > >
> > > > Hi,
> > > >
> > > > Assuming this Sling Model (using Lombok's @Getter annotation)
> > > >
> > > > @Getter
> > > > @Model(
> > > > adaptables = { SlingHttpServletRequest.class },
> > > > adapters = { MyModel.class, ComponentExporter.class },
> > > > resourceType = MyModel.RESOURCE_TYPE) @Exporter(
> > > > name = ExporterConstants.SLING_MODEL_EXPORTER_NAME,
> > > > extensions = ExporterConstants.SLING_MODEL_EXTENSION)
> > > > public class MyModel implements ComponentExporter {
> > > >
> > > > static final String RESOURCE_TYPE =
> "myapp/components/mymodel";
> > > >
> > > > @Inject
> > > > private ResourceResolver resolver;
> > > >
> > > > @ChildResource
> > > > private List items;
> > > >
> > > > }
> > > >
> > > > When it this model is serialized via SlingModelExporter / Jackson,
> the
> > > > resolver field is also exported via the created getResolver())
> method.
> > > >
> > > > But serializing that does not always work:
> > > >
> > > > org.apache.sling.models.factory.ExportException:
> > > > com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No
> > > > serializer found for class
> > > > com.day.cq.wcm.core.impl.policies.ContentPolicyManagerImpl and no
> > > > properties discovered to create BeanSerializer (to avoid exception,
> > > > disable
> > > > SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain:
> > > > com.myapp.PageImpl[":items"]> [...] > com.myapp.MyModel["resolver"]
> > > >
> > >org.apache.sling.resourceresolver.impl.ResourceResolverImpl["propertyMa
> > > > >p"]
> > > >
> > >java.util.HashMap["com.day.cq.wcm.core.impl.policies.ContentPolicyAdapt
> > > > >erFactory.ContentPolicy"])
> > > > at
> > > >
> > >
> >
> org.apache.sling.models.jacksonexporter.impl.JacksonExporter.export(Jackso
> > > > nExporter.java:138)
> > > > [org.apache.sling.models.jacksonexporter:1.1.2]
> > > > at
> > > >
> > >
> >
> org.apache.sling.models.impl.ModelAdapterFactory.exportModel(ModelAdapterF
> > > > actory.java:1333)
> > > > [org.apache.sling.models.impl:1.5.4]
> > > >
> > > >
> > 

Re: Sling Model Caching & GC problems

[Paul Bjorkstrand]

Hi Jörg,

My guess is that you are running up against the problem where the Model is
referencing its adaptable, directly or indirectly. In that situation, the
model would not be collectable because it is referenced more strongly than
by weak reference. The reference path of these might look like this:

Model Cache Holder (the Model Adapter Factory
(strong reference)
Model Cache
(soft reference)
Model
(strong reference)
Resource Resolver
(strong reference, maybe indirectly)
Resource [the adaptable]


The resource is strongly or softly referenced, possibly indirectly, making
it ineligible for collection on normal GC cycles. The resource & resolver,
will not be collected until after the model is collected. Since the model
is not collected until there is memory pressure, that would explain why you
are seeing this (expensive) GC behavior.

When memory pressure occurs, first the models (soft references) are
collected prior to the OOM, which releases both resolver (no longer
referenced via field in the model) and resource.

The quickest fix is to release the resource resolver at the end of the
model’s constructor or @PostConstruct [2]. Alternatively, you can eliminate
the cache=true, provided it does not negatively impact your application
performance.
Another option, though more involved, is that the entire caching impl could
be changed to better handle these kinds of reference loops, by putting the
cache in the adaptable itself (Resolver, Resource, Request, etc). Possible
good candidates of these are [4] or request attributes. [4] seems to be the
overall best candidate, especially since you can hook into it using
Closeable ([5]) improving the cache eviction even more.

As long as the object holding the cache is not referenced strongly outside
that reference loop (cache holder > cache > model > ... > cache holder),
then the loop's objects would be eligible for GC as soon as the cache
holder is eligible.

[1]:
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/SoftReference.html
[2]:
https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector
[3]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/18
[4]:
https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888
[5]:
https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L610

// Paul


On Tue, Jan 23, 2024 at 6:48 AM Jörg Hoh 
wrote:

> Hi Sling community,
>
> I want to share a recent experience I had with Sling Models, Sling Model
> caching and Garbage Collection problems.
>
> I had a case, where an AEM instance had massive garbage collection
> problems, but no memory problems. We saw the regular sawtooth pattern in
> the heap consumption, but heavy GC activity (in a stop-the-world manner)
> almost constantly. But no OutOfMemory situation, there it's not a memory
> leak.
>
> I manually captured a heapdump and found a lot of Sling models being
> referenced by the Sling ModelAdapterFactory cache, and rechecking these
> model classes in detail I found them to specify "cache=true" in their
> @Model annotation.When these statements were removed, the situation looks
> completely different, and the garbage collection was normal again.
>
> I don't have a full explanation for this behavior yet. The Sling Models had
> a reference to a ResourceResolver (which was properly closed), but I assume
> that this reference somehow "disabled" the cleaning of the cache on major
> GCs (as its a WeakHashMap), but tied the collection of these models to the
> collection of the ResourceResolver objects, which have finalizers
> registered. And finalizers are only executed under memory pressure. Having
> this connetion might have led to the situation that the SlingModel objects
> were not disposed eagerly, but only alongside the finalizers in situation
> of high memory pressure in a "stop-the-world" situation.
>
> I try to get some more examples for that behavior; but I am not sure of the
> caching of Sling Models as-is is something we should continue to use. This
> case was quite hard to crack, and I don't know if/how we can avoid such a
> situation by design.
>
> Jörg
>
> --
> https://cqdump.joerghoh.de
>

Re: [Vote] Release Apache Sling Engine 2.15.12

[Paul Bjorkstrand]

-1 (non-binding)

I have a concern with one change that was merged recently. I added a
comment, but it was already merged by the time I read through it. I think a
recent log addition [1] is incorrect. I think this logging addition will
not log what is desired.

[1]:
https://github.com/apache/sling-org-apache-sling-engine/pull/41/files#diff-6457a924a51a1c233c4c4c45de49a05b0e438032ac111c0c1644c6dcd546ab9bR521

// Paul


On Thu, Apr 18, 2024 at 6:08 AM Stefan Seifert
 wrote:

> +1
>
> stefan
>

Re: Sling Model Caching & GC problems

[Paul Bjorkstrand]

I didn't see any (public) activity on this, and I had some time to look at
this over this weekend. Here's the ticket [1] and PR [2] that would move
the cache for Resource/Resolver based models into the ResourceResovler
property map. Interestingly, request adaptables already had a similar type
of cache colocation. I just modified the adapter factory's servlet listener
to call close() on the cache when the request object is destroyed. If the
adaptable somehow is neither a Request, Resource, nor Resolver, then the
original caching implementation is preserved.

This change would also likely close [3] as they are directly related.

Give it a look and additional testing as needed.

[1]: https://issues.apache.org/jira/browse/SLING-12279
[2]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/50
[3]: https://issues.apache.org/jira/browse/SLING-12259

// Paul


On Tue, Jan 30, 2024 at 6:36 AM Carsten Ziegeler 
wrote:

> Sure, no problem with using a WeakHashMap.
>
> Regards
> Carsten
>
> On 30.01.2024 10:28, Jörg Hoh wrote:
> > Paul,
> >
> > thanks for your mail. I agree to your observation what's going on. I also
> > think that cache at the ResourceResolver level makes more sense, and it's
> > easier to control and remove cached models, which cannot be used any
> more.
> >
> > @Carsten: Regarding the size, we can still use a WeakHashMap as a
> container
> > for these Sling Models (and store the WeakHashMap in the PropertyMap of
> the
> > ResourceResolver), and that should prevent the worst, even in case of a
> > long-running ResourceResolver or many adaptions.
> >
> > Jörg
> >
> >
> > Am Di., 30. Jan. 2024 um 07:16 Uhr schrieb Carsten Ziegeler <
> > cziege...@apache.org>:
> >
> >> I don't know much about sling models caching, but it seems storing the
> >> cache into the resource resolver and therefore binding it to that
> >> lifecycle sounds very reasonable to me. And due to the mentioned support
> >> for Closeable, the cache gets discarded automatically with the resolver
> >> being closed. No extra handling required, no soft references etc.
> needed.
> >>
> >> Obviously, that sounds problematic with long or forever running resource
> >> resolvers as the cache would never be released. However, I guess thats
> >> not worse than what we have today. And long running resource resolvers
> >> are an anti-pattern anyway.
> >>
> >> Regards
> >> Carsten
> >>
> >> On 23.01.2024 17:23, Paul Bjorkstrand wrote:
> >>> Hi Jörg,
> >>>
> >>> My guess is that you are running up against the problem where the Model
> >> is
> >>> referencing its adaptable, directly or indirectly. In that situation,
> the
> >>> model would not be collectable because it is referenced more strongly
> >> than
> >>> by weak reference. The reference path of these might look like this:
> >>>
> >>> Model Cache Holder (the Model Adapter Factory
> >>> (strong reference)
> >>> Model Cache
> >>> (soft reference)
> >>> Model
> >>> (strong reference)
> >>> Resource Resolver
> >>> (strong reference, maybe indirectly)
> >>> Resource [the adaptable]
> >>>
> >>>
> >>> The resource is strongly or softly referenced, possibly indirectly,
> >> making
> >>> it ineligible for collection on normal GC cycles. The resource &
> >> resolver,
> >>> will not be collected until after the model is collected. Since the
> model
> >>> is not collected until there is memory pressure, that would explain why
> >> you
> >>> are seeing this (expensive) GC behavior.
> >>>
> >>> When memory pressure occurs, first the models (soft references) are
> >>> collected prior to the OOM, which releases both resolver (no longer
> >>> referenced via field in the model) and resource.
> >>>
> >>> The quickest fix is to release the resource resolver at the end of the
> >>> model’s constructor or @PostConstruct [2]. Alternatively, you can
> >> eliminate
> >>> the cache=true, provided it does not negatively impact your application
> >>> performance.
> >>> Another option, though more involved, is that the entire caching impl
> >> could
> >>> be changed to better handle these kinds of reference loops, by putting
> >> the
> >>> cache in the adaptable itself (Resolver, Resource, Request, etc).
> >> Possible
> >>> go

Re: Sling Model Caching & GC problems

[Paul Bjorkstrand]

Oops, in that last line I meant to say "then the original caching behavior
is preserved." instead of "then the original caching implementation is
preserved."

// Paul


On Sat, Mar 30, 2024 at 4:35 PM Paul Bjorkstrand 
wrote:

> I didn't see any (public) activity on this, and I had some time to look at
> this over this weekend. Here's the ticket [1] and PR [2] that would move
> the cache for Resource/Resolver based models into the ResourceResovler
> property map. Interestingly, request adaptables already had a similar type
> of cache colocation. I just modified the adapter factory's servlet listener
> to call close() on the cache when the request object is destroyed. If the
> adaptable somehow is neither a Request, Resource, nor Resolver, then the
> original caching implementation is preserved.
>
> This change would also likely close [3] as they are directly related.
>
> Give it a look and additional testing as needed.
>
> [1]: https://issues.apache.org/jira/browse/SLING-12279
> [2]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/50
> [3]: https://issues.apache.org/jira/browse/SLING-12259
>
> // Paul
>
>
> On Tue, Jan 30, 2024 at 6:36 AM Carsten Ziegeler 
> wrote:
>
>> Sure, no problem with using a WeakHashMap.
>>
>> Regards
>> Carsten
>>
>> On 30.01.2024 10:28, Jörg Hoh wrote:
>> > Paul,
>> >
>> > thanks for your mail. I agree to your observation what's going on. I
>> also
>> > think that cache at the ResourceResolver level makes more sense, and
>> it's
>> > easier to control and remove cached models, which cannot be used any
>> more.
>> >
>> > @Carsten: Regarding the size, we can still use a WeakHashMap as a
>> container
>> > for these Sling Models (and store the WeakHashMap in the PropertyMap of
>> the
>> > ResourceResolver), and that should prevent the worst, even in case of a
>> > long-running ResourceResolver or many adaptions.
>> >
>> > Jörg
>> >
>> >
>> > Am Di., 30. Jan. 2024 um 07:16 Uhr schrieb Carsten Ziegeler <
>> > cziege...@apache.org>:
>> >
>> >> I don't know much about sling models caching, but it seems storing the
>> >> cache into the resource resolver and therefore binding it to that
>> >> lifecycle sounds very reasonable to me. And due to the mentioned
>> support
>> >> for Closeable, the cache gets discarded automatically with the resolver
>> >> being closed. No extra handling required, no soft references etc.
>> needed.
>> >>
>> >> Obviously, that sounds problematic with long or forever running
>> resource
>> >> resolvers as the cache would never be released. However, I guess thats
>> >> not worse than what we have today. And long running resource resolvers
>> >> are an anti-pattern anyway.
>> >>
>> >> Regards
>> >> Carsten
>> >>
>> >> On 23.01.2024 17:23, Paul Bjorkstrand wrote:
>> >>> Hi Jörg,
>> >>>
>> >>> My guess is that you are running up against the problem where the
>> Model
>> >> is
>> >>> referencing its adaptable, directly or indirectly. In that situation,
>> the
>> >>> model would not be collectable because it is referenced more strongly
>> >> than
>> >>> by weak reference. The reference path of these might look like this:
>> >>>
>> >>> Model Cache Holder (the Model Adapter Factory
>> >>> (strong reference)
>> >>> Model Cache
>> >>> (soft reference)
>> >>> Model
>> >>> (strong reference)
>> >>> Resource Resolver
>> >>> (strong reference, maybe indirectly)
>> >>> Resource [the adaptable]
>> >>>
>> >>>
>> >>> The resource is strongly or softly referenced, possibly indirectly,
>> >> making
>> >>> it ineligible for collection on normal GC cycles. The resource &
>> >> resolver,
>> >>> will not be collected until after the model is collected. Since the
>> model
>> >>> is not collected until there is memory pressure, that would explain
>> why
>> >> you
>> >>> are seeing this (expensive) GC behavior.
>> >>>
>> >>> When memory pressure occurs, first the models (soft references) are
>> >>> collected prior to the OOM, which releases both resolver (no longer
>> >>> referenced via 

[jira] [Created] (SLING-7549) tag is not being treated as "self closing" when used in conjunction with HTL block statements

[Paul Bjorkstrand (JIRA)]

Paul Bjorkstrand created SLING-7549:
---

 Summary:  tag is not being treated as "self closing" when 
used in conjunction with HTL block statements
 Key: SLING-7549
 URL: https://issues.apache.org/jira/browse/SLING-7549
 Project: Sling
  Issue Type: Bug
  Components: Scripting
Affects Versions: Scripting HTL Engine 1.0.32
Reporter: Paul Bjorkstrand


When using HTL block statements some tags that are assumed to be "self closing" 
by browsers are not handled in that manner by the HTL engine. This can cause 
hard to find problems with the rendering of the page. in this example, the 
{{}} tag was the culprit. 

I was diagnosing an issue where under a certain set of circumstances all of my 
page's css was not being rendered. This happened to be due a {{}} tag 
that was not closed, but had a block statement added to it:
{code:java}
{code}
That seemingly innocuous tag caused the HTL engine to assume that all contents 
following, (until the the parent head closing tag), to be inside that link tag.

According to the [html5 
spec|https://www.w3.org/TR/2014/REC-html5-20141028/document-metadata.html#the-link-element]
 the content of the {{}} element is empty. Nearly every browser in use 
today treats {{}} the same as {{}}. Given this, it is not 
a stretch to assume that HTL would treat it in the same manner.

It is my belief that HTL should mimic the html5 specification. At minimum, I 
would hope to get a clear answer on which HTML specification it follows, or a 
statement that HTL does not/will not follow an HTML specification.

*Note*: I was unsure if this should be something in the HTL spec or in the 
implementation. Since I found no reference in the specification regarding 
html5/xhtml/html4 compliance, I came here with my request instead. Please let 
me know if this is something that should be addressed at the specification 
level.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors

[Paul Bjorkstrand (JIRA)]

Paul Bjorkstrand created SLING-8069:
---

 Summary: Sling Models: Enable constructor injection to use 
non-public constructors
 Key: SLING-8069
 URL: https://issues.apache.org/jira/browse/SLING-8069
 Project: Sling
  Issue Type: Improvement
Affects Versions: Sling Models Impl 1.4.10
Reporter: Paul Bjorkstrand


In Sling Models, you cannot use a non-public constructor to inject. Looking 
through the code, there doesn't seem to be any clear reason for this 
restriction. In my opinion, constructor injection should allow any constructor 
visibility.

Here are some discussion points:
 * Private fields are allowed for use, so disallowing private constructors 
seems unnecessary.
 * Private constructors may be bad practice (difficult to test), but Sling 
should not be telling users how to write their Java code. This is especially 
true for models, since it should work with POJOs, as stated in the 
documentation. It would be trivial to add checks to just allow default, 
protected, or public, but I feel that logic is unnecessary.
 * Non-public methods could also be allowed, but that can be a separate ticket.
 ** A prerequisite of this would be to allow setter injection on models in the 
first place. Again, not the subject of this ticket.
 * Threading concerns are minimal, but there could be possible deadlocks, as 
with any multi-threaded application that uses locks. In general, I think 
locking similar to how it is done in {{InjectableField}} would be sufficient. 
The risk of deadlock would be similar to the risk of the locking in 
{{injectableField.set(Object, Result)}}.

 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors

[Paul Bjorkstrand (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-8069?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16673606#comment-16673606
 ] 

Paul Bjorkstrand commented on SLING-8069:
-

Added Github PR link since it didn't link automatically

> Sling Models: Enable constructor injection to use non-public constructors
> -
>
> Key: SLING-8069
> URL: https://issues.apache.org/jira/browse/SLING-8069
> Project: Sling
>  Issue Type: Improvement
>Affects Versions: Sling Models Impl 1.4.10
>    Reporter: Paul Bjorkstrand
>Priority: Minor
>
> In Sling Models, you cannot use a non-public constructor to inject. Looking 
> through the code, there doesn't seem to be any clear reason for this 
> restriction. In my opinion, constructor injection should allow any 
> constructor visibility.
> Here are some discussion points:
>  * Private fields are allowed for use, so disallowing private constructors 
> seems unnecessary.
>  * Private constructors may be bad practice (difficult to test), but Sling 
> should not be telling users how to write their Java code. This is especially 
> true for models, since it should work with POJOs, as stated in the 
> documentation. It would be trivial to add checks to just allow default, 
> protected, or public, but I feel that logic is unnecessary.
>  * Non-public methods could also be allowed, but that can be a separate 
> ticket.
>  ** A prerequisite of this would be to allow setter injection on models in 
> the first place. Again, not the subject of this ticket.
>  * Threading concerns are minimal, but there could be possible deadlocks, as 
> with any multi-threaded application that uses locks. In general, I think 
> locking similar to how it is done in {{InjectableField}} would be sufficient. 
> The risk of deadlock would be similar to the risk of the locking in 
> {{injectableField.set(Object, Result)}}.
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM

[Paul Bjorkstrand (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16846018#comment-16846018
 ] 

Paul Bjorkstrand commented on SLING-8425:
-

Thanks for the assist [~radu.cotescu]. I can confirm that the latest fixes work 
on GraalVM.

> NPE in SlingScriptEngineManager when Sling is run on GraalVM
> 
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>    Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting Core 2.0.58
>
>  Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the 
> SlingScriptEngineManager when it tried to call 
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}. 
> The problem is that a script engine provided by Graal (Nashorn, in this case) 
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into 
> it yet), but Sling can be defensive, and not call the method(s) inside 
> {{registerAssociations}} when it sees a null value for either an extension, 
> mime type, or name.
> Fixing this issue also exposes another issue: the 
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a 
> single built-in scripting engine. In Graal, there could be two (or more) 
> built-in scripting engines. In my situation, there were two: GraalJS and 
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still 
> runs through the registration process, so the tests need to account for it.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM

[Paul Bjorkstrand (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16844221#comment-16844221
 ] 

Paul Bjorkstrand commented on SLING-8425:
-

I found it equally weird. I opened 
[https://github.com/oracle/graal/issues/1310] which I hope will get some 
attention. If I had to guess why it is this way, when they implemented GraalJS, 
instead of modifying Nashorn extensively to remove its reported properties' 
values (extensions, mime types, and names), they "cheated" and null-fill an 
array somewhere in Nashorn at runtime.

I'll try to keep an eye on that ticket for any movement. If I don't hear back 
in a reasonable amount of time (any thoughts of what would be "reasonable" 
would be appreciated), would it make sense to move this forward and protect 
Sling from the choices of JDK implementors? 

> NPE in SlingScriptEngineManager when Sling is run on GraalVM
> 
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
>  Time Spent: 10m
>  Remaining Estimate: 0h
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the 
> SlingScriptEngineManager when it tried to call 
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}. 
> The problem is that a script engine provided by Graal (Nashorn, in this case) 
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into 
> it yet), but Sling can be defensive, and not call the method(s) inside 
> {{registerAssociations}} when it sees a null value for either an extension, 
> mime type, or name.
> Fixing this issue also exposes another issue: the 
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a 
> single built-in scripting engine. In Graal, there could be two (or more) 
> built-in scripting engines. In my situation, there were two: GraalJS and 
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still 
> runs through the registration process, so the tests need to account for it.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM

[Paul Bjorkstrand (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-8425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16844047#comment-16844047
 ] 

Paul Bjorkstrand commented on SLING-8425:
-

I have a proposed fix in the works, and will submit a PR later today.

> NPE in SlingScriptEngineManager when Sling is run on GraalVM
> 
>
> Key: SLING-8425
> URL: https://issues.apache.org/jira/browse/SLING-8425
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Core 2.0.54
> Environment: OS: Ubuntu 18.04.2 LTS
> JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
>    Reporter: Paul Bjorkstrand
>Priority: Major
>
> When trying to run Sling Starter 11 on GraalVM, there was an NPE in the 
> SlingScriptEngineManager when it tried to call 
> {{internalManager.registerEngineExtension}} inside {{registerAssociations}}. 
> The problem is that a script engine provided by Graal (Nashorn, in this case) 
> had {{null}} as an extension value.
> I imagine that it is a bug with GraalVM itself (I have not dug further into 
> it yet), but Sling can be defensive, and not call the method(s) inside 
> {{registerAssociations}} when it sees a null value for either an extension, 
> mime type, or name.
> Fixing this issue also exposes another issue: the 
> SlingScriptEngineManagerTest assumes that the JDK it is running on only has a 
> single built-in scripting engine. In Graal, there could be two (or more) 
> built-in scripting engines. In my situation, there were two: GraalJS and 
> Nashorn. Even though Nashorn is a seemingly-broken engine in Graal, it still 
> runs through the registration process, so the tests need to account for it.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (SLING-8425) NPE in SlingScriptEngineManager when Sling is run on GraalVM

[Paul Bjorkstrand (JIRA)]

Paul Bjorkstrand created SLING-8425:
---

 Summary: NPE in SlingScriptEngineManager when Sling is run on 
GraalVM
 Key: SLING-8425
 URL: https://issues.apache.org/jira/browse/SLING-8425
 Project: Sling
  Issue Type: Bug
  Components: Scripting
Affects Versions: Scripting Core 2.0.54
 Environment: OS: Ubuntu 18.04.2 LTS
JVM: OpenJDK GraalVM CE 19.0.0 (build 25.212-b03-jvmci-19-b01, mixed mode)
Reporter: Paul Bjorkstrand


When trying to run Sling Starter 11 on GraalVM, there was an NPE in the 
SlingScriptEngineManager when it tried to call 
{{internalManager.registerEngineExtension}} inside {{registerAssociations}}. 
The problem is that a script engine provided by Graal (Nashorn, in this case) 
had {{null}} as an extension value.

I imagine that it is a bug with GraalVM itself (I have not dug further into it 
yet), but Sling can be defensive, and not call the method(s) inside 
{{registerAssociations}} when it sees a null value for either an extension, 
mime type, or name.

Fixing this issue also exposes another issue: the SlingScriptEngineManagerTest 
assumes that the JDK it is running on only has a single built-in scripting 
engine. In Graal, there could be two (or more) built-in scripting engines. In 
my situation, there were two: GraalJS and Nashorn. Even though Nashorn is a 
seemingly-broken engine in Graal, it still runs through the registration 
process, so the tests need to account for it.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (SLING-9314) HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() returns null

[Paul Bjorkstrand (Jira)]

Paul Bjorkstrand created SLING-9314:
---

 Summary: HTL null pointer in ObjectModel.toBoolean(Object) when 
object.toString() returns null
 Key: SLING-9314
 URL: https://issues.apache.org/jira/browse/SLING-9314
 Project: Sling
  Issue Type: Bug
  Components: Scripting
Affects Versions: Scripting HTL Runtime 1.1.2-1.4.0, Scripting HTL Runtime 
1.1.0-1.4.0, Scripting HTL Runtime 1.0.0-1.4.0
Reporter: Paul Bjorkstrand


Though it is bad practice, it is possible that an object can return null from 
its toString() method. ObjectModel.toBoolean(Object) \[[Line 
161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\]
 calls .trim() on a potentially null object.

This causes a difficult to troubleshoot, deeply nested, and cryptic exception 
to be raised. If object.toString() returns null, then it should be treated the 
same as nearly the rest of HTL, where null is considered "falsey". Doing so 
will save hours of difficult troubleshooting.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (SLING-9715) Sling models JavaUseProvider does not properly handle the adaptable argument for Sling Model classes

[Paul Bjorkstrand (Jira)]

Paul Bjorkstrand created SLING-9715:
---

 Summary: Sling models JavaUseProvider does not properly handle the 
adaptable argument for Sling Model classes
 Key: SLING-9715
 URL: https://issues.apache.org/jira/browse/SLING-9715
 Project: Sling
  Issue Type: Bug
  Components: Sling Models
Affects Versions: Scripting HTL Engine 1.4.0-1.4.0
Reporter: Paul Bjorkstrand


When Sling Models instantiation was added in SLING-9320 
([commit|https://github.com/apache/sling-org-apache-sling-scripting-sightly/commit/bcd46027e09182911c55bb244d42debb1bd367c7]),
 it did not treat the {{adaptable}} argument the same for all types of 
instantiations (Sling Models vs basic Sling Adaptable). This fixes that by 
rearranging the instantiation attempt order, which puts the argument provided 
adaptable through {{ModelFactory#createModel}} just like the request and 
resource would be.

This is a subtle semantic change, and likely warrants a minor version update.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Comment Edited] (SLING-9829) data-sly-element should correctly handle void elements

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17214717#comment-17214717
 ] 

Paul Bjorkstrand edited comment on SLING-9829 at 10/15/20, 2:00 PM:


Though it would be bad form to add children to an eventual void element, I am 
curious how you will handle when the element name is for a void element, but 
the actual element has child nodes (text content or other elements). It might 
be worthwhile to at least debug log when that situation occurs, something along 
the lines of "The name in data-sly-element  is a void element, 
all children of this element will be ignored/removed"

Thoughts?


was (Author: paul.bjorkstrand):
Though it would be bead form, I am curious how you will handle when the element 
name is for a void element, but the actual element has child nodes (text 
content or other elements). It might be worthwhile to at least debug log when 
that situation occurs, something along the lines of "The name in 
data-sly-element  is a void element, all children of this element 
will be ignored/removed"

Thoughts?

> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler 
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
>
> The current implementation of {{data-sly-element}} doesn't correctly handle 
> void elements which will replace the original element on which the block 
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element 
> will have a closing tag in the following example, although both {{link}} and 
> {{meta}} are void elements:
> {code:html}
> 
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-9829) data-sly-element should correctly handle void elements

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17214717#comment-17214717
 ] 

Paul Bjorkstrand commented on SLING-9829:
-

Though it would be bead form, I am curious how you will handle when the element 
name is for a void element, but the actual element has child nodes (text 
content or other elements). It might be worthwhile to at least debug log when 
that situation occurs, something along the lines of "The name in 
data-sly-element  is a void element, all children of this element 
will be ignored/removed"

Thoughts?

> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler 
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
>
> The current implementation of {{data-sly-element}} doesn't correctly handle 
> void elements which will replace the original element on which the block 
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element 
> will have a closing tag in the following example, although both {{link}} and 
> {{meta}} are void elements:
> {code:html}
> 
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-9314) HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() returns null

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17184103#comment-17184103
 ] 

Paul Bjorkstrand commented on SLING-9314:
-

That works too, [~radu]. This was mainly a "quick get the details down" kind of 
report, that I did not yet follow up on. The suggestion for falsey was based on 
my specific situation. It might be worthwhile to add a warn/error log when 
toString returns null.

As I stated in the first sentence, it is bad practice (and ostensibly breaks 
the contract of {{#toString}}) but that does not stop bad code from sneaking 
in. Thinking for both allowing further processing and helping with 
troubleshooting something like this maybe (pseudocode):

{code}
s = o.toString();

if (s == null) {
 log warn or error with as much detail as possible
 s = EMPTY
}

s = s.trim()
... the rest of the logic ...
{code}

> HTL null pointer in ObjectModel.toBoolean(Object) when object.toString() 
> returns null
> -
>
> Key: SLING-9314
> URL: https://issues.apache.org/jira/browse/SLING-9314
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime 
> 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0
>Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
>
> Though it is bad practice, it is possible that an object can return null from 
> its toString() method. ObjectModel.toBoolean(Object) \[[Line 
> 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\]
>  calls .trim() on a potentially null object.
> This causes a difficult to troubleshoot, deeply nested, and cryptic exception 
> to be raised. If object.toString() returns null, then it should be treated 
> the same as nearly the rest of HTL, where null is considered "falsey". Doing 
> so will save hours of difficult troubleshooting.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Comment Edited] (SLING-9314) NPE in ObjectModel.toBoolean(Object) when object.toString() returns null

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185209#comment-17185209
 ] 

Paul Bjorkstrand edited comment on SLING-9314 at 8/26/20, 2:56 PM:
---

[~radu], I added a comment (rather lengthy, sorry) to the PR with an idea on 
how to both simplify the logic and to make it more readable. There is one 
non-backwards compatible change in the suggestion, though I feel it is worth 
eliminating that edge case. Let me know your thoughts on the impacts of that 
change.


was (Author: paul.bjorkstrand):
[~radu], I add a comment (rather lengthy, sorry) to the PR with an idea on how 
to both simplify the logic and to make it more readable. There is one 
non-backwards compatible change in the suggestion, though I feel it is worth 
eliminating that edge case. Let me know your thoughts on the impacts of that 
change.

> NPE in ObjectModel.toBoolean(Object) when object.toString() returns null
> 
>
> Key: SLING-9314
> URL: https://issues.apache.org/jira/browse/SLING-9314
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime 
> 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0
>    Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Runtime 1.2.4-1.4.0
>
>
> Though it is bad practice, it is possible that an object can return null from 
> its toString() method. ObjectModel.toBoolean(Object) \[[Line 
> 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\]
>  calls .trim() on a potentially null object.
> This causes a difficult to troubleshoot, deeply nested, and cryptic exception 
> to be raised. If object.toString() returns null, then it should be treated 
> the same as nearly the rest of HTL, where null is considered "falsey". Doing 
> so will save hours of difficult troubleshooting.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-9314) NPE in ObjectModel.toBoolean(Object) when object.toString() returns null

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185209#comment-17185209
 ] 

Paul Bjorkstrand commented on SLING-9314:
-

[~radu], I add a comment (rather lengthy, sorry) to the PR with an idea on how 
to both simplify the logic and to make it more readable. There is one 
non-backwards compatible change in the suggestion, though I feel it is worth 
eliminating that edge case. Let me know your thoughts on the impacts of that 
change.

> NPE in ObjectModel.toBoolean(Object) when object.toString() returns null
> 
>
> Key: SLING-9314
> URL: https://issues.apache.org/jira/browse/SLING-9314
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting HTL Runtime 1.0.0-1.4.0, Scripting HTL Runtime 
> 1.1.0-1.4.0, Scripting HTL Runtime 1.1.2-1.4.0
>    Reporter: Paul Bjorkstrand
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Runtime 1.2.4-1.4.0
>
>
> Though it is bad practice, it is possible that an object can return null from 
> its toString() method. ObjectModel.toBoolean(Object) \[[Line 
> 161|https://github.com/apache/sling-org-apache-sling-scripting-sightly-runtime/blob/master/src/main/java/org/apache/sling/scripting/sightly/render/ObjectModel.java#L161]\]
>  calls .trim() on a potentially null object.
> This causes a difficult to troubleshoot, deeply nested, and cryptic exception 
> to be raised. If object.toString() returns null, then it should be treated 
> the same as nearly the rest of HTL, where null is considered "falsey". Doing 
> so will save hours of difficult troubleshooting.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17142358#comment-17142358
 ] 

Paul Bjorkstrand commented on SLING-8706:
-

I agree with [~justinedelson] on the HTL side. Just because Sling Models 
supports this feature doesn't make it required to be supported in HTL (though, 
only one supporting with the other not supporting {{Optional}} could lead to 
some confusion). I also have never seen any (Java) serialization of Sling 
Models, so I don't see that as a barrier either. Besides, serialization [can be 
customized|https://howtodoinjava.com/java/serialization/custom-serialization-readobject-writeobject/]
 ([in more than one 
way|https://dzone.com/articles/how-to-customize-serialization-in-java-using-the-e])
 to work around non-serializable types already.

Based on the intended use of {{Optional}} (to be used at API boundaries aka 
method returns), this I feel is a gray area. While a field isn't technically 
part of an API, they are injected by some external process. One could argue 
that the fields are equivalent to the return values of that external process. 
In that argument, it makes sense to capture that intent in the type system 
itself, to capture the fact that "this field could be null, so you need to 
check it". This alone makes me think this is a reasonable approach to handling 
optional injections.

That being said, there is nothing stopping a developer from improperly using an 
{{Optional}}, say, by using {{.get()}} without first checking {{.isPresent()}}, 
in turn causing a NPE. While bad practice, yes, it happens frequently enough 
that I almost always have a conversation with my teams on proper {{Optional}} 
usage. I still believe this idea is reasonable, but I don't buy into the 
"optional will prevent NPEs" argument.

> Injections for java.util.Optional<> should be automatic optional 
> -
>
> Key: SLING-8706
> URL: https://issues.apache.org/jira/browse/SLING-8706
> Project: Sling
>  Issue Type: Improvement
>  Components: Sling Models
>Reporter: Jörg Hoh
>Priority: Major
>  Time Spent: 2h 40m
>  Remaining Estimate: 0h
>
> The current approach to support optional injections requires to annotate the 
> field with {{@Optional}} plus proper handling within the javacode (null 
> checks etc), which can be forgotten.
> So instead of
> {code}
> @Inject @Optional
> String fieldname;
> {code}
> it should also be possible to use this
> {code}
> @Inject
> Optional fieldname;
> {code}
> with the very same semantic. But the developer is forced to deal with the 
> case that the value is not present.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-10011) Use javax.jcr.Item.getParent() when resolving parent JCR node in JcrResourceProvider#getParent

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10011?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17253778#comment-17253778
 ] 

Paul Bjorkstrand commented on SLING-10011:
--

I think the point that [~cziegeler] and [~jsedding] are making is that the 
session _should_ have enough information to optimize the call
{code:java}
((JackrabbitSession) session).getItemOrNull(path);{code}
which is made in [JcrResourceitemFactory, line 187 (at time of 
writing)|https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/master/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java#L187].
 If an improvement is needed, it should be made in Oak for two reasons:
 # Sling should not need to use a specific call to get performance from the JCR 
(except in unusual edge cases).
 # If an improvement is made in Oak, then all projects that use Oak benefit, 
including Sling.

While that may increase the effort of this, it puts the effort in the correct 
location: at the database.

Think of it in terms analogous to RDBMS: you would not expect to use one type 
of SQL query if the database was split across many disks, and another if it 
were on a single SSD. You would expect the database to have knowledge of its 
logical and physical structure, so it can optimize its own operations. That is 
the abstraction a database user would expect.

> Use javax.jcr.Item.getParent() when resolving parent JCR node in 
> JcrResourceProvider#getParent
> --
>
> Key: SLING-10011
> URL: https://issues.apache.org/jira/browse/SLING-10011
> Project: Sling
>  Issue Type: Improvement
>  Components: JCR
>Affects Versions: JCR Resource 3.0.22
>Reporter: Miroslav Smiljanic
>Priority: Minor
>  Time Spent: 20m
>  Remaining Estimate: 0h
>
> Currently 
> [JcrResourceProvider.getParent|https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/org.apache.sling.jcr.resource-3.0.22/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L361]
>  is using JcrItemResourceFactory.getItemOrNull(String path), which eventually 
> is using JCR session to retrieve parent node using absolute path.
> I propose using javax.jcr.Item.getParent() instead.
> Reasoning wold be to utilise potential improvements in JCR implementation 
> that would for a given node retrieve the whole subtree. That can be 
> configured for example by using particular node type or node path.
> {noformat}
> root
>  |
>  a 
>/   \
>   b c
> {noformat}
> If node 'a' in picture above, is matching desired configuration, then code 
> below would return the whole subtree.
> {code:java}
> Node a = jcrSession.getNode("a");
> {code}
> That further means retrieved subtree can be traversed in memory, without the 
> need to communicate with the JCR repository storage.
> (!)That is particularly important when remote (cloud) storage is used for 
> repository in JCR implementation, and tree traversal can be done without 
> doing additional network roundtrips.
> {code:java}
> //JCR tree traversal happens in memory
> Node b = a.getNode("b");
> Node c = a.getNode("c");
> {code}
> Also going from child to parent, is resolved in memory as well (proposal 
> relates to this fact)
> {code:java}
> //JCR tree traversal happens in memory
> assert b.getParent() == c.getParent();
> {code}
> Jackrabbit Oak, for document node store is supporting node bundling for 
> configured node type
>  [http://jackrabbit.apache.org/oak/docs/nodestore/document/node-bundling.html]
> Currently I am also doing some experiments to support node 
> bundling/aggregation for arbitrary node store 
> ([NodeDelegateFullyLoaded|https://github.com/smiroslav/jackrabbit-oak/blob/ppnextgen_newstore/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegateFullyLoaded.java],
>  
> [FullyLoadedTree|https://github.com/smiroslav/jackrabbit-oak/blob/ppnextgen_newstore/oak-core/src/main/java/org/apache/jackrabbit/oak/core/FullyLoadedTree.java]).



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-9829) data-sly-element should correctly handle void elements

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-9829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17219080#comment-17219080
 ] 

Paul Bjorkstrand commented on SLING-9829:
-

Thanks for the explanation. Even if it were just the element name, that would 
be helpful. I understand that it would be nontrivial, but it would probably be 
useful to add in some kind of logging facility in the future. I added 
SLING-9849 to request that feature.

 

Thanks again, [~radu].

> data-sly-element should correctly handle void elements
> --
>
> Key: SLING-9829
> URL: https://issues.apache.org/jira/browse/SLING-9829
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Affects Versions: Scripting Sightly Engine 1.0.0, Scripting HTL Compiler 
> 1.0.0, Scripting HTL Compiler 1.1.0-1.4.0, Scripting HTL Compiler 1.2.0-1.4.0
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: Scripting HTL Compiler 1.2.12-1.4.0
>
>  Time Spent: 20m
>  Remaining Estimate: 0h
>
> The current implementation of {{data-sly-element}} doesn't correctly handle 
> void elements which will replace the original element on which the block 
> element was placed:
> When {{$\{item.element.name}}} evaluates to {{link}} or {{meta}}, the element 
> will have a closing tag in the following example, although both {{link}} and 
> {{meta}} are void elements:
> {code:html}
> 
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Created] (SLING-9849) Add logging functionality to the compiler and runtime to support better error notification for the developer

[Paul Bjorkstrand (Jira)]

Paul Bjorkstrand created SLING-9849:
---

 Summary: Add logging functionality to the compiler and runtime to 
support better error notification for the developer
 Key: SLING-9849
 URL: https://issues.apache.org/jira/browse/SLING-9849
 Project: Sling
  Issue Type: Wish
  Components: Scripting
Reporter: Paul Bjorkstrand


As a developer, it would be helpful to have more detailed logging in compiled 
HTL scripts to assist with troubleshooting issues in the HTL script logic.

SLING-9829 exposed the fact that there is no runtime support for logging errors 
or warnings with any degree of specificity.

To implement this would likely require (at minimum), changes to the HTL 
compiler and HTL runtime.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-10113) data-sly-resource throwing RecursionTooDeepException

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10113?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17276645#comment-17276645
 ] 

Paul Bjorkstrand commented on SLING-10113:
--

Does {{targetPagePath}} start with a slash ({{/}})? Per the code [1], the path 
is appended to the current resource, if it does not


[1]: 
https://github.com/apache/sling-org-apache-sling-scripting-sightly/blob/org.apache.sling.scripting.sightly-1.1.2-1.4.0/src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/ResourceRuntimeExtension.java#L212

> data-sly-resource throwing RecursionTooDeepException
> 
>
> Key: SLING-10113
> URL: https://issues.apache.org/jira/browse/SLING-10113
> Project: Sling
>  Issue Type: Bug
>  Components: Scripting
>Reporter: Marvin Flock
>Priority: Major
> Attachments: image-2021-02-01-17-40-19-873.png
>
>
> AEM Version 6.5.7
> While using data-sly-resource with a path, I get a RecursionTooDeepException.
>  See -SLING-7685-  for Similarity
> I currently cant make hold of the behavior, using
> __
>  will include the current page infinite times (at least it tries) instead of 
> the target page
>  while using something like this:
>  __
>  will render the curentPage with the RecursionTooDeepException.
> The HTL Engine I am using is of the following version:
>  !image-2021-02-01-17-40-19-873.png!



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-10150) Sling Resource Merger completely hides parent when whitelisting in combination with asterisk is used for sling:hideChildren

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10150?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17287319#comment-17287319
 ] 

Paul Bjorkstrand commented on SLING-10150:
--

Just a question for [~Henry Kuijpers], have you tried changing the order of the 
hide-children values? If the {{!desired-tab}} is evaluated first then the {{*}} 
is next, maybe it is an ordering problem (or maybe I am completely missing part 
of the problem).

> Sling Resource Merger completely hides parent when whitelisting in 
> combination with asterisk is used for sling:hideChildren
> ---
>
> Key: SLING-10150
> URL: https://issues.apache.org/jira/browse/SLING-10150
> Project: Sling
>  Issue Type: Bug
>Affects Versions: Resource Merger 1.3.10
>Reporter: Henry Kuijpers
>Priority: Major
>  Time Spent: 10m
>  Remaining Estimate: 0h
>
> Provided a failing test case here: 
> https://github.com/apache/sling-org-apache-sling-resourcemerger/pull/3/files
> TODO: Fix
> When trying to configure a whitelisting approach to inheriting nodes from a 
> parent (i.e. through resource super type, or through overlaying), the 
> following way:
> /apps/base/test/tabs/undesired-tab
> /apps/base/test/tabs/desired-tab
> /apps/base/test/tabs/desired-tab/items/field@title="title"
> &
> /apps/overlay/test/tabs@sling:hideChildren="[!desired-tab,*]"
> /apps/overlay/test/tabs/desired-tab/items/field@description="test"
> One would expect that requesting the children of /merged/test/tabs would 
> yield the "desired-tab" only, i.e. "undesired-tab" (and other nodes not 
> whitelisted) being hidden. This is working as expected.
> One would also expect the "desired-tab" to have the properties of the 
> base-structure as well as the properties of the overlay-structure. This is 
> also working as expected.
> One would expect that the underlying nodes of "desired-tab" from the base 
> would remain intact and would be merged with the underlying nodes of 
> "desired-tab" in the overlay. So, while listing the items of desired-tab, one 
> would expect:
> MergedResource containing properties [title=test, description=test] 
> consisting of original resources 
> [/apps/base/test/tabs/desired-tab/items/field, 
> /apps/overlay/test/tabs/desired-tab/items/field]
> However, instead, the following is returned:
> MergedResource containing properties [description=test] consisting of 
> original resources [/apps/overlay/test/tabs/desired-tab/items/field]
> So, the original "base" resource is not considered anymore!
> I believe the issue is in MergingResourceProvider.ParentHidingHandler, in the 
> constructor, actually. At some point, it decides that the parent resource 
> (which indeed has the sling:hideChildren property defined) defines an exclude 
> entry which is "*" and adds that entry to the list.
> Then, when the class starts checking the parent of the parent (marked with 
> "// also check on the parent's parent whether that was hiding the parent") - 
> There it will find that asterisk exclude that was defined on the parent, not 
> taking into account that it was preceded by a whitelisting "!desired-tab" - 
> Removing the parent of the parent's children entirely.
> I believe this should be changed into a more robust way of handling this 
> use-case. Probably the asterisk exclude can be global(?), even though it 
> should still be desired that any child of the parent still is able to remove 
> that exclude. But whenever those excludes are considered, also the includes 
> that were preceding it should be considered to figure out if it's a real 
> include in the case of that specific path.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[jira] [Commented] (SLING-10969) Remove synchronized & rest of accessible flag during injection

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10969?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17454090#comment-17454090
 ] 

Paul Bjorkstrand commented on SLING-10969:
--

[~sseifert], [~rombert], [~kwin]

PR: https://github.com/apache/sling-org-apache-sling-models-impl/pull/29

> Remove synchronized & rest of accessible flag during injection
> --
>
> Key: SLING-10969
> URL: https://issues.apache.org/jira/browse/SLING-10969
> Project: Sling
>  Issue Type: Improvement
>  Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Paul Bjorkstrand
>Priority: Major
>
> To improve multithreaded performance of Sling Models, the {{synchronized}} 
> blocks, along with the "reset" of the {{setAccessible}} using its original 
> value should be removed.
> Context:
> The synchronized blocks were added to resolve a race condition [1]. After 
> looking into another large project that uses reflection to access members of 
> classes (Felix [2]), nowhere in that project is something similar being done. 
> Every reflective access is just doing {{setAccessible(true)}}.
> Results from a JMH test allude to a significant performance improvement 
> during multithreaded threaded access by removing the synchronized (JMH 
> results [3]).
> [1]: SLING-6584
> [2]: https://github.com/apache/felix-dev/search?q=setAccessible
> [3]: https://gist.github.com/paul-bjorkstrand/f3bb154665e7d2168b4656eb7b794496
> [4]: https://www.mail-archive.com/dev@sling.apache.org/msg113123.html
> [5]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/11



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Created] (SLING-10969) Remove synchronized & rest of accessible flag during injection

[Paul Bjorkstrand (Jira)]

Paul Bjorkstrand created SLING-10969:


 Summary: Remove synchronized & rest of accessible flag during 
injection
 Key: SLING-10969
 URL: https://issues.apache.org/jira/browse/SLING-10969
 Project: Sling
  Issue Type: Improvement
  Components: Sling Models
Affects Versions: Models Implementation 1.4.16
Reporter: Paul Bjorkstrand


To improve multithreaded performance of Sling Models, the {{synchronized}} 
blocks, along with the "reset" of the {{setAccessible}} using its original 
value should be removed.

Context:
The synchronized blocks were added to resolve a race condition [1]. After 
looking into another large project that uses reflection to access members of 
classes (Felix [2]), nowhere in that project is something similar being done. 
Every reflective access is just doing {{setAccessible(true)}}.

Results from a JMH test allude to a significant performance improvement during 
multithreaded threaded access by removing the synchronized (JMH results [3]).

[1]: SLING-6584
[2]: https://github.com/apache/felix-dev/search?q=setAccessible
[3]: https://gist.github.com/paul-bjorkstrand/f3bb154665e7d2168b4656eb7b794496
[4]: https://www.mail-archive.com/dev@sling.apache.org/msg113123.html
[5]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/11



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-10946) slingsettings deprecated what should i now use?

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10946?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17450780#comment-17450780
 ] 

Paul Bjorkstrand commented on SLING-10946:
--

[~paul@gmail.com], the 
[SlingSettingsService|https://github.com/apache/sling-org-apache-sling-settings/blob/master/src/main/java/org/apache/sling/settings/SlingSettingsService.java]
 is not deprecated as far as I am aware. I imagine you are talking about 
[AEMaaCS' version of that 
API|https://javadoc.io/doc/com.adobe.aem/aem-sdk-api/latest/org/apache/sling/settings/SlingSettingsService.html]
 being deprecated. That is not part of Apache Sling, though.

Regardless, this is not the proper venue for the question. If it is an AEM 
specific question, I recommend Adobe's Experience League community. Also, you 
can join us on the [mailing 
lists|https://sling.apache.org/project-information.html#mailing-lists] for 
Apache Sling related questions.

Cheers!

> slingsettings deprecated what should i now use?
> ---
>
> Key: SLING-10946
> URL: https://issues.apache.org/jira/browse/SLING-10946
> Project: Sling
>  Issue Type: Bug
>  Components: Sling Models
>Reporter: Paul W
>Priority: Minor
>
> not necessarily a bug, but now that slingsettings is deprecated, we are not 
> able to get the runmode in the sling model, what is the alternative to this?



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451275#comment-17451275
 ] 

Paul Bjorkstrand commented on SLING-10947:
--

I had an inspiration, so this evening became this morning :)

https://github.com/apache/sling-org-apache-sling-models-impl/pull/25

It looks like there is a sonar issue 
(https://ci-builds.apache.org/blue/organizations/jenkins/Sling%2Fmodules%2Fsling-org-apache-sling-models-impl/detail/PR-25/1/pipeline/33/),
 that seems to be unrelated to the changes. Can you take a look [~sseifert] (or 
if you know who can, bring 'em in to the conversation)?

> Sling Models Unit Tests do not compile with Java 11
> ---
>
> Key: SLING-10947
> URL: https://issues.apache.org/jira/browse/SLING-10947
> Project: Sling
>  Issue Type: Bug
>  Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Stefan Seifert
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
>
> due to a reference to sun.misc in 
> https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451277#comment-17451277
 ] 

Paul Bjorkstrand commented on SLING-10947:
--

I just noticed, this ticket is somewhat a duplicate of SLING-10037., maybe we 
can close this one in favor of that one?

> Sling Models Unit Tests do not compile with Java 11
> ---
>
> Key: SLING-10947
> URL: https://issues.apache.org/jira/browse/SLING-10947
> Project: Sling
>  Issue Type: Bug
>  Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Stefan Seifert
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
>
> due to a reference to sun.misc in 
> https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-10947) Sling Models Unit Tests do not compile with Java 11

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-10947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17451242#comment-17451242
 ] 

Paul Bjorkstrand commented on SLING-10947:
--

Having been the one to write that test, I think we can refactor it to avoid the 
need to use {{Unsafe}}. Lemme mull on it for the day and I'll see if I can get 
an MR up this evening.

> Sling Models Unit Tests do not compile with Java 11
> ---
>
> Key: SLING-10947
> URL: https://issues.apache.org/jira/browse/SLING-10947
> Project: Sling
>  Issue Type: Bug
>  Components: Sling Models
>Affects Versions: Models Implementation 1.4.16
>Reporter: Stefan Seifert
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
>
> due to a reference to sun.misc in 
> https://github.com/apache/sling-org-apache-sling-models-impl/blob/a016b05184a17f3760f8594139fabc573d5bfdae/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java#L313-L335



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-8069) Sling Models: Enable constructor injection to use non-public constructors

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-8069?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17459254#comment-17459254
 ] 

Paul Bjorkstrand commented on SLING-8069:
-

Absolutely, [~kwin]. PR coming shortly for sling-site.

> Sling Models: Enable constructor injection to use non-public constructors
> -
>
> Key: SLING-8069
> URL: https://issues.apache.org/jira/browse/SLING-8069
> Project: Sling
>  Issue Type: Improvement
>  Components: Sling Models
>Affects Versions: Models Impl 1.4.10
>Reporter: Paul Bjorkstrand
>Assignee: Konrad Windszus
>Priority: Minor
> Fix For: Models Implementation 1.5.0
>
>  Time Spent: 2h
>  Remaining Estimate: 0h
>
> In Sling Models, you cannot use a non-public constructor to inject. Looking 
> through the code, there doesn't seem to be any clear reason for this 
> restriction. In my opinion, constructor injection should allow any 
> constructor visibility.
> Here are some discussion points:
>  * Private fields are allowed for use, so disallowing private constructors 
> seems unnecessary.
>  * Private constructors may be bad practice (difficult to test), but Sling 
> should not be telling users how to write their Java code. This is especially 
> true for models, since it should work with POJOs, as stated in the 
> documentation. It would be trivial to add checks to just allow default, 
> protected, or public, but I feel that logic is unnecessary.
>  * Non-public methods could also be allowed, but that can be a separate 
> ticket.
>  ** A prerequisite of this would be to allow setter injection on models in 
> the first place. Again, not the subject of this ticket.
>  * Threading concerns are minimal, but there could be possible deadlocks, as 
> with any multi-threaded application that uses locks. In general, I think 
> locking similar to how it is done in {{InjectableField}} would be sufficient. 
> The risk of deadlock would be similar to the risk of the locking in 
> {{injectableField.set(Object, Result)}}.
>  



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839116#comment-17839116
 ] 

Paul Bjorkstrand commented on SLING-12300:
--

I'm leaning towards [~joerghoh]'s opinion as well. To me, this seems better 
served by a new method like {{ResourceResolver#getResoureById(String jcrId);}}

Having this functionality in {{getResource(..)}} method seems to be a possible 
source for confusion.

If I were to add in something that provides resources under the path 
{{/jcr:id/*}}, it almost seems like it should be from a provider that services 
that path. That may also bring into it more problems, because I don't know if 
it is semantically correct for a resource provider under a given path to 
provide resources for another path.

Is there a reason why this can't be a new method?

> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
>  Issue Type: New Feature
>  Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would 
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}} 
> property for referenceable nodes or the path. In systems that would like to 
> use UUID addressing, this would reduce the need for executing JCR queries for 
> resource retrieval and would avoid double-reads via the JCR and then Sling 
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}} 
> prefix should use the resource retrieval by node identifier.
> [0] - 
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17840127#comment-17840127
 ] 

Paul Bjorkstrand commented on SLING-12300:
--

That's a good analysis, [~radu], thanks for sharing. I understand the Resource 
API challenges. It makes sense to keep this as a JCR concept. I feel we can 
drop that idea from contention given its significant constraints.

Regarding the other two, maybe there is a way to be able to have the best of 
both worlds, by making the new {{/jcr:id/}} path available, but behind a 
configuration flag ({{false}} by default). I think that would allow you to keep 
the simpler implementation, but also addresses any security/exposure related 
issues, since it would be an opt-in feature.

Thoughts?

> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
>  Issue Type: New Feature
>  Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would 
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}} 
> property for referenceable nodes or the path. In systems that would like to 
> use UUID addressing, this would reduce the need for executing JCR queries for 
> resource retrieval and would avoid double-reads via the JCR and then Sling 
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}} 
> prefix should use the resource retrieval by node identifier.
> [0] - 
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839397#comment-17839397
 ] 

Paul Bjorkstrand commented on SLING-12300:
--

To clarify my position:

I am not against having a mechanism to access them via identifier. My argument 
is mostly around the 'magic" path of {{/jcr:id/*}}. To me, I feel using a new 
method (as mentioned above) to perform this function. If you are creating an 
application that works outside the typical Sling hierarchical retrieve, it 
should probably be a new primitive in Sling, and not comingled with the 
existing {{getResource()}}.

An argument for this functionality  might be: Sling already have vanity paths 
that muddy this concept, so what is the concern for another?

About the security argument, I think it is fair to say that we all know that 
"security by obscurity" is no security at all. Just because there is some means 
to access nodes by some ID that has a smaller universe of unique identifiers 
compared to the path does not change any of the security 
characteristics...unless you are using obscurity as your security mechanism.

As Radu stated, the security checks performed are still the same ones that 
would be performed on the target node/resource, so there is no real security 
weakening here.

While I still think that adding the magic path is still a possible source of 
confusion, I am not so against it after some deeper thinking on it. I still 
believe that a new method would be better, but it seems reasonable, especially 
when compared side-by-side with vanities.

Would creating a new resource provider actually duplicate a lot? Or would you 
be able to build this as an additional provider in the same bundle as the JCR 
Resource Provider, and use the same functionality directly?

> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
>  Issue Type: New Feature
>  Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would 
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}} 
> property for referenceable nodes or the path. In systems that would like to 
> use UUID addressing, this would reduce the need for executing JCR queries for 
> resource retrieval and would avoid double-reads via the JCR and then Sling 
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}} 
> prefix should use the resource retrieval by node identifier.
> [0] - 
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841341#comment-17841341
 ] 

Paul Bjorkstrand commented on SLING-12279:
--

[~joerghoh], can you take a look at this and see if it solves your problem? I 
believe it will, but I don't have a way to reliably test. I could try to 
contrive some kind of unit test that does some verification, though it may 
prove difficult to make. Since the GC is largely non-deterministic (from an 
outsider's perspective), getting this into a field test would likely be better.

> Move Sling Model cache holder for Resource and ResourceResolver adaptables 
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
>  Issue Type: Improvement
>  Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>    Reporter: Paul Bjorkstrand
>Priority: Major
>
> There have been several instances of issues with model caching over the 
> years, the most recent being SLING-12259 and [this 
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from 
>  Jörg Hoh. These recent issues have been around cached items sticking 
> around for too long. In that thread, it was discussed using 
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
>  whenever possible. This binds the cache to the lifecycle of the 
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current 
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
>  there is already a divergence that binds the cache for request-based models 
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and 
> use the resolver's property map to bind the cache for those adaptables to the 
> lifecycle of the ResourceResolver. Resources are already (largely) bound to 
> the lifecycle of the ResourceResolver that supplied them, and binding the 
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the 
> likelihood of the JVM's GC being put under pressure in cases when cached 
> models [reference the original adaptable using 
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
>  As a bonus, if the cache object implements {{Closeable}}, the Sling 
> Implementation will call {{close()}} on the cache when the resolver is 
> closed,, giving us further control over the lifetime of objects in the cache.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (SLING-12300) Provide a way to retrieve a JCR backed resource by its node identifier

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17839419#comment-17839419
 ] 

Paul Bjorkstrand commented on SLING-12300:
--

I apologize if I am perceived as trivializing; I am trying to understand your 
concern, but I am not able to find any reason (other than security/privacy) why 
having the UUIDs addressable is a problem. I do want to understand what the 
concern about having some kind of predictability is, though.

Based on part of your previous comments, I would like to address some of the 
concerns strictly related to predictability.

bq. Basically, I'm not putting a server on the public internet with predictable 
addresses.

I don't think that predictability is necessarily a problem in itself. In many 
situations (especially for things based on Sling), predictability is not a 
detriment but a feature. I know that there are Sling implementations that are 
not AEM, but using AEM as an example, you already have a large swath of 
partially predictable addresses. Public data primarily lives under 
{{/content}}. (Mostly private) user data lives under {{/home}}, (almost 
entirely private) application data/code lives under {{/apps}}. These root 
segments are entirely predictable.

If you will oblige, I would like to run through an example comparing UUIDs vs. 
paths in terms of predictability, starting with some assumptions:

# The "root path segments" in a given implementation are entirely predictable.
# Paths consist of characters found in the following regex: {{[A-Za-z0-9-/]}}. 
This gives you 64 characters to choose from (more or fewer characters could be 
used, and these seem common enough in URLs).
#* I chose this character set because it made the math easier, since 64 is a 
power of 2.
#* More characters could be used, but it doesn't really change the math much.
# Paths of nodes are entirely random, using the character set above.
# Every node in a given Sling instance has {{mix:referenceable}} applied and is 
provisioned a UUID.
# Paths could be any random combination of the characters from the regex above.

In theory, a path could be of unlimited length, while UUIDs are finite, that is 
true. Using the assumptions above, for a path to be less predictable than a 
UUID, it would need to be at least than 21 characters long _beyond any 
predictable root(s)_. I won't go into the math too deeply, but the point where 
64^x^ crosses 2^122^ is approximately 20.41. Anything 20 characters or less is 
more prone to brute force attacks than a UUID!

_Note, even if you use all 8 bits of ASCII, you get 256^x^, which makes the 
length needed 16 characters (~15.25).

In practice, paths are almost never random strings of characters. They have 
meaning in their names, often semantic and syntactic restrictions (depending on 
the creator's language). System rules can also reduce what paths are allowed 
(e.g., {{//}} is not legal because of the sequential slashes). Additionally, 
paths are usually relatively short. My experience tells me that paths are 
rarely more than 50 characters long in the public part of the application. 
Lastly, not every node in a given instance is going to have 
{{mix:referenceable}}, and thus not every node has a UUID that could be 
brute-forced.

If your concern is not security or privacy related can help me understand what 
it is [~enorman]?

> Provide a way to retrieve a JCR backed resource by its node identifier
> --
>
> Key: SLING-12300
> URL: https://issues.apache.org/jira/browse/SLING-12300
> Project: Sling
>  Issue Type: New Feature
>  Components: JCR
>Reporter: Radu Cotescu
>Assignee: Radu Cotescu
>Priority: Major
> Fix For: JCR Resource 3.3.0
>
>
> Since all {{javax.jcr.Nodes}} have an identifier [0], a useful feature would 
> be {{Resource}} retrieval by node id, which could be its {{jcr:uuid}} 
> property for referenceable nodes or the path. In systems that would like to 
> use UUID addressing, this would reduce the need for executing JCR queries for 
> resource retrieval and would avoid double-reads via the JCR and then Sling 
> API to obtain the resource.
> In order to provide a unified behaviour, paths starting with the {{/jcr:id/}} 
> prefix should use the resource retrieval by node identifier.
> [0] - 
> https://javadoc.io/static/javax.jcr/jcr/2.0/javax/jcr/Node.html#getIdentifier()



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map

[Paul Bjorkstrand (Jira)]

[ 
https://issues.apache.org/jira/browse/SLING-12279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17846767#comment-17846767
 ] 

Paul Bjorkstrand commented on SLING-12279:
--

[~pakira], you are correct that this does not fix that particular problem. The 
problem that this attempts to address, is when the cache fills with 
[circularly-referenced cached 
models|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector]
 (most commonly when model marked with `cache=true`, and keeps a field with 
`@Self`), this change should allow the GC to collect the cache when the 
Resource Resolver or Request are closed/destroyed. That will eliminate a large 
"sawtooth" GC graph.

Without this change, models that, directly or indirectly, reference their 
adaptable will not be collected until there is memory pressure, and the GC 
starts collecting SoftReference. I do believe with my proposed change, swapping 
out the cache impl (currently just a `Map, 
SoftReference>>` will be easier. A follow-on change could add in the 
LRU portion of the cache impl in one place, rather than in the several it would 
need today..

> Move Sling Model cache holder for Resource and ResourceResolver adaptables 
> into the resource resolver property map
> --
>
> Key: SLING-12279
> URL: https://issues.apache.org/jira/browse/SLING-12279
> Project: Sling
>  Issue Type: Improvement
>  Components: Sling Models
>Affects Versions: Models Implementation 1.6.4
>Reporter: Paul Bjorkstrand
>Priority: Major
> Attachments: cache-size.log, cachesize.log
>
>
> There have been several instances of issues with model caching over the 
> years, the most recent being SLING-12259 and [this 
> thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from 
>  Jörg Hoh. These recent issues have been around cached items sticking 
> around for too long. In that thread, it was discussed using 
> {{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
>  whenever possible. This binds the cache to the lifecycle of the 
> ResourceResolver, avoiding the global cache altogether.
> After looking at the [current 
> implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
>  there is already a divergence that binds the cache for request-based models 
> to the request, by putting the cache in the request attribute.
> This proposes to do a similar change for Resource/Resolver based models, and 
> use the resolver's property map to bind the cache for those adaptables to the 
> lifecycle of the ResourceResolver. Resources are already (largely) bound to 
> the lifecycle of the ResourceResolver that supplied them, and binding the 
> models that come from these Resources seems to be a reasonable approach.
> This will greatly reduce the lifetime of model objects, reducing the 
> likelihood of the JVM's GC being put under pressure in cases when cached 
> models [reference the original adaptable using 
> {{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
>  As a bonus, if the cache object implements {{Closeable}}, the Sling 
> Implementation will call {{close()}} on the cache when the resolver is 
> closed,, giving us further control over the lifetime of objects in the cache.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Created] (SLING-12279) Move Sling Model cache holder for Resource and ResourceResolver adaptables into the resource resolver property map

[Paul Bjorkstrand (Jira)]

Paul Bjorkstrand created SLING-12279:


 Summary: Move Sling Model cache holder for Resource and 
ResourceResolver adaptables into the resource resolver property map
 Key: SLING-12279
 URL: https://issues.apache.org/jira/browse/SLING-12279
 Project: Sling
  Issue Type: Improvement
  Components: Sling Models
Affects Versions: Models Implementation 1.6.4
Reporter: Paul Bjorkstrand


There have been several instances of issues with model caching over the years, 
the most recent being SLING-12259 and [this 
thread|https://www.mail-archive.com/dev@sling.apache.org/msg131926.html] from   
 Jörg Hoh. These recent issues have been around cached items sticking 
around for too long. In that thread, it was discussed using 
{{[ResourceResolver#getPropertyMap()|https://github.com/apache/sling-org-apache-sling-api/blob/master/src/main/java/org/apache/sling/api/resource/ResourceResolver.java#L888]}}
 whenever possible. This binds the cache to the lifecycle of the 
ResourceResolver, avoiding the global cache altogether.

After looking at the [current 
implementation|https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L341],
 there is already a divergence that binds the cache for request-based models to 
the request, by putting the cache in the request attribute.

This proposes to do a similar change for Resource/Resolver based models, and 
use the resolver's property map to bind the cache for those adaptables to the 
lifecycle of the ResourceResolver. Resources are already (largely) bound to the 
lifecycle of the ResourceResolver that supplied them, and binding the models 
that come from these Resources seems to be a reasonable approach.

This will greatly reduce the lifetime of model objects, reducing the likelihood 
of the JVM's GC being put under pressure in cases when cached models [reference 
the original adaptable using 
{{@Self}}|https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector].
 As a bonus, if the cache object implements {{Closeable}}, the Sling 
Implementation will call {{close()}} on the cache when the resolver is closed,, 
giving us further control over the lifetime of objects in the cache.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)