JCRVLT-517 fix package cache init in OSGi containers #136

kwin · 2021-04-28T19:36:50Z

get rid of lazy-loading of package cache in packages()
improve test coverage
fix another issue in FSInstallState not correctly persisting excludes

get rid of lazy-loading of package cache in packages() improve test coverage fix another issue in FSInstallState not correctly persisting excludes

tripodsan

@kwin I'm not comfortable with this change, I think we added the lazy loading for a reason. so that bundle activation is fast.....

kwin · 2021-04-29T05:02:34Z

Any reason why lazy loading was not used in the other constructor?

tripodsan · 2021-04-29T05:18:37Z

Any reason why lazy loading was not used in the other constructor?

which other constructor?

kwin · 2021-04-29T06:11:33Z

which other constructor?

jackrabbit-filevault/vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSPackageRegistry.java

Line 166 in 7eb85af

loadPackageCache();

Also lazy loading is not at all implemented correctly. It needs to be done IMHO on almost all operations (not only on packages and contains but also all register method which should throw exceptions in case of duplicate package ids). Lazy loading also requires proper synchronization, as multiple methods may be called from different threads. Also bundle activation is not deferred by long-running component activators.

Why do you think that long-running activate() methods in DS are a problem?

tripodsan

ok then :-)

stoerr · 2021-04-29T06:31:48Z

I did consider getting rid of lazy loading completely when doing #135 , but I was afraid the original author might have had a good reason to do it. The problem is that the method loadPackages() is a quite expensive method, as it accesses the filesystem and opens all files to collect metadata. If the filesystem is a network filesystem (which might even make sense for distributing packages across instances), then this can even block indefinitely. That's fine in tests (at least within Jackrabbit Vault the FSPackageRegistryIT and your new test seem to be the only places the additional constructors are used), but that might not be OK during OSGI activation. Could that block your bundles from coming up? And the registry might even not be used at all later...

You could consider encapsulating stateCache to switch it completely to lazy loading to be consistent (in the original code getInstallState / setInstallState methods also didn't call the loadPackages()), but that does change the behavior of the public constructors somewhat, so it might theoretically break e.g. a test someone did in a project using Vault, though that's admittedly a strange case. Not sure whether it's worth the risk.

stoerr · 2021-04-29T06:33:00Z

...ore/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSPackageRegistry.java

@@ -185,6 +182,7 @@ public void activate(BundleContext context, Config config) {
        log.info("Jackrabbit Filevault FS Package Registry initialized with home location {}", this.homeDir.getPath());
        this.scope = InstallationScope.valueOf(config.scope());
        this.securityConfig = new AbstractPackageRegistry.SecurityConfig(config.authIdsForHookExecution(), config.authIdsForRootInstallation());
+        loadPackageCache();


I think doing file system accesses within the OSGI activation is dangerous - please see my comment #136 (comment) .

The reason for not loading the cache here is because package operations are no frequent use case while the activate happens at any start of the system. This was designed for a Cloud-Service scenario with frequent starts & stops. Any expensive operation should be cut out of the startup procedure. The non-osgi initialization on the other hand is only required for initialization of an 'immutable' state of this is registry which would then only be read at system runtime (reused state) to validate dependencies for the mutable part being handled by the jcr registry. In case we have new scenarios we should make sure to not sacrifice those performance aspects.

Clarification - installation of packages happens with an osgi startup at the moment as other mechanisms may write to repositorylike repo init but the accessible containers would only initialize the cache when there is package installation activity which is a rare scenario and is not supposed to delay startup

Loading the package cache is crucial before running ExecutionPlanBuilder (look at https://github.com/apache/sling-org-apache-sling-jcr-packageinit/pull/7/files). But I will come up with a fix which prevents reading the state from FileSystem in case the FSPackageRegistry is just referenced but no methods are called (as it is the case sometimes in https://github.com/apache/sling-org-apache-sling-jcr-packageinit/blob/master/src/main/java/org/apache/sling/jcr/packageinit/impl/ExecutionPlanRepoInitializer.java

stoerr · 2021-04-29T06:34:50Z

...t-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSInstallState.java

+
+    @Override
+    public int hashCode() {
+        final int prime = 31;


Just for fun: I always thought using 31 as a factor is a quite bad choice, as it easily creates conflicts. https://stackoverflow.com/a/2816747/21499 explains why I think 92821 is much better when you are using that pattern. :-)

kwin · 2021-04-29T06:36:24Z

I just checked again and the default timeout in Felix SCR for activate methods is 5 seconds (https://github.com/apache/felix-dev/blob/3e5671ae7e5107f4f849ef9d5f0a89b1ba9d7439/scr/src/main/java/org/apache/felix/scr/impl/manager/ScrConfiguration.java#L60), there are other alternatives outlined at https://stackoverflow.com/a/57250631.

stoerr · 2021-04-29T06:42:45Z

...t-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSInstallState.java

+
+    @Override
+    public String toString() {
+        return "FSInstallState [" + (packageId != null ? "packageId=" + packageId + ", " : "")


Yes! I like it when you're adding toString() wherever it makes sense. That's often more pleasant in the debugger.

stoerr · 2021-04-29T06:53:02Z

While we're at it: there are some log messages in FSPackageRegistry that could log the file / package they apply to to make it easier to find which of the potentially gazillion packages they apply to. I don't find a way to add a comment to these lines here in the PR since there are no changes near those, but I would suggest extending the log messages at lines 471, 581 (also typo in NOTREGISTERD), 684, 695, 704, 727 with some information about the package concerned. Thank you so much!

DominikSuess

Please don't introduce costly synchronous processing in the osgi startup as package activities are rather a rare scenario in contrast to system start-up in containerized Cloud-Service setups.

tripodsan

given the comments from the others, please remove the package loading from the activation method.

reintroduce lazy loading refactor tests

sonarcloud · 2021-05-04T15:54:28Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

58.0% Coverage
0.0% Duplication

tripodsan

I can't really judge the changes... but I like the decoupling of the install state.

kwin requested a review from tripodsan April 28, 2021 19:36

kwin mentioned this pull request Apr 28, 2021

JCRVLT-517 FSPackageRegistry.contains does not initialize packages #135

Closed

JCRVLT-517 fix package cache init in OSGi containers

0364b48

get rid of lazy-loading of package cache in packages() improve test coverage fix another issue in FSInstallState not correctly persisting excludes

kwin force-pushed the bugfix/JCRVLT-517-init-package-cache-in-osgi branch from 717e0ea to 0364b48 Compare April 28, 2021 19:38

tripodsan reviewed Apr 29, 2021

View reviewed changes

tripodsan approved these changes Apr 29, 2021

View reviewed changes

stoerr reviewed Apr 29, 2021

View reviewed changes

DominikSuess suggested changes Apr 29, 2021

View reviewed changes

tripodsan suggested changes Apr 29, 2021

View reviewed changes

kwin force-pushed the bugfix/JCRVLT-517-init-package-cache-in-osgi branch from c4df25c to dc5fdd5 Compare May 4, 2021 10:10

JCRVLT-517 extract cache into dedicated class

e16c467

reintroduce lazy loading refactor tests

kwin force-pushed the bugfix/JCRVLT-517-init-package-cache-in-osgi branch from dc5fdd5 to e16c467 Compare May 4, 2021 10:12

kwin requested a review from tripodsan May 4, 2021 10:13

kwin added 3 commits May 4, 2021 12:15

fix typo

7b8108e

add missing license header

b01476b

fix handling of non-external files

5c42f14

tripodsan approved these changes May 5, 2021

View reviewed changes

kwin merged commit 3dba5a2 into master May 5, 2021

kwin deleted the bugfix/JCRVLT-517-init-package-cache-in-osgi branch May 5, 2021 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JCRVLT-517 fix package cache init in OSGi containers #136

JCRVLT-517 fix package cache init in OSGi containers #136

kwin commented Apr 28, 2021

tripodsan left a comment

kwin commented Apr 29, 2021

tripodsan commented Apr 29, 2021

kwin commented Apr 29, 2021

tripodsan left a comment

stoerr commented Apr 29, 2021 •

edited

stoerr Apr 29, 2021

DominikSuess Apr 29, 2021

DominikSuess Apr 29, 2021

kwin Apr 29, 2021 •

edited

stoerr Apr 29, 2021

kwin commented Apr 29, 2021

stoerr Apr 29, 2021

stoerr commented Apr 29, 2021

DominikSuess left a comment

tripodsan left a comment

sonarcloud bot commented May 4, 2021

tripodsan left a comment

JCRVLT-517 fix package cache init in OSGi containers #136

JCRVLT-517 fix package cache init in OSGi containers #136

Conversation

kwin commented Apr 28, 2021

tripodsan left a comment

Choose a reason for hiding this comment

kwin commented Apr 29, 2021

tripodsan commented Apr 29, 2021

kwin commented Apr 29, 2021

tripodsan left a comment

Choose a reason for hiding this comment

stoerr commented Apr 29, 2021 • edited

stoerr Apr 29, 2021

Choose a reason for hiding this comment

DominikSuess Apr 29, 2021

Choose a reason for hiding this comment

DominikSuess Apr 29, 2021

Choose a reason for hiding this comment

kwin Apr 29, 2021 • edited

Choose a reason for hiding this comment

stoerr Apr 29, 2021

Choose a reason for hiding this comment

kwin commented Apr 29, 2021

stoerr Apr 29, 2021

Choose a reason for hiding this comment

stoerr commented Apr 29, 2021

DominikSuess left a comment

Choose a reason for hiding this comment

tripodsan left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 4, 2021

tripodsan left a comment

Choose a reason for hiding this comment

stoerr commented Apr 29, 2021 •

edited

kwin Apr 29, 2021 •

edited