joerghoh commented on PR #89: URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/89#issuecomment-6058809605
I found two correctness concerns with the cache enabled. The first is reproduced by additional lifecycle assertions in the PR’s test infrastructure; the second is reproduced with a mocked registry visibility change. ### 1. Injector deactivation can release service objects that models still use `OSGiServiceInjector.deactivate()` closes the cache and releases its service usages. However, models holding those injected objects may still be in use. This matters for a `ServiceFactory` that disposes resources when a bundle releases its last usage. A Java reference to the injected object does not preserve its OSGi usage count. Releasing the cache’s sole usage can therefore dispose the object while a model still holds it. There are two affected scenarios: - **An already-created model:** deactivating the injector releases the cached usage, potentially invalidating the model’s injected service. - **Acquisition concurrent with deactivation:** `OSGiServiceCache.getService()` obtains an object, detects that the cache was closed, calls `ungetService()`, and then returns that same object. Injection can consequently return an already-disposed resource. The relevant scenario is live injector reconfiguration, not necessarily application shutdown: the service provider can remain registered and the model bundle can remain active throughout. Additional assertions expose both scenarios. The uncached control passes because its service usage remains associated with the model’s disposal callback. The existing usage-count assertions do not detect this problem: zero remaining usages demonstrates successful release, but does not demonstrate that an injected object remains usable. Cache shutdown needs to distinguish stopping new cache acquisitions from releasing objects still used by models. ### 2. Changes to `FindHook` visibility do not invalidate affected cached lookups `OSGiServiceCache.serviceChanged()` invalidates lookups only for the classes listed in the changed service’s `objectClass` property. Registering or unregistering a `FindHook` therefore invalidates lookups for the hook itself, but not for the service classes whose visibility it changes. For example: 1. A lookup caches a reference to service `S`. 2. A `FindHook` is registered that hides `S` from the requesting bundle. 3. Subsequent injections use the cached reference without consulting the hook, so `S` remains injectable. The reverse is also possible: an empty result cached while a hook hides a service can remain empty after removing that hook. The stale result persists until another event invalidates the target service class. Hook lifecycle changes therefore need broader lookup invalidation. That alone does not address stateful hooks that change their filtering decisions without a service event; those require an explicit compatibility policy, such as bypassing the lookup cache in environments using dynamic visibility hooks. The configuration documentation should also explain any intentional limitations. Currently, enabling the cache appears to be a lookup optimization, but it can change which services are visible to models. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
