[
https://issues.apache.org/jira/browse/SLING-13259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18094332#comment-18094332
]
Joerg Hoh commented on SLING-13259:
-----------------------------------
That feels a bit incomplete to me, because if just one PropertyProvider is
supposed to be stopped (the Sling instance itself is not going to be shut
down), this approach won't help.
I wonder why on unbind of a PropertyProvider {{doUpdateProperties()}} is called
at all. Or if we can do the call to {{doUpdateProperties()}} in an async way
and moving it off the critical thread.
> OakDiscoveryService can block framework shutdown when Oak CommitQueue is slow
> during PropertyProvider unbind
> -------------------------------------------------------------------------------------------------------------
>
> Key: SLING-13259
> URL: https://issues.apache.org/jira/browse/SLING-13259
> Project: Sling
> Issue Type: Bug
> Components: Discovery
> Reporter: Daniel Iancu
> Priority: Major
>
> h2. Description
> h3. What happens
> During OSGi framework shutdown, the Felix start-level thread can block for
> many minutes — potentially indefinitely — inside
> {{{}OakDiscoveryService.doUpdateProperties(){}}}, waiting for a JCR commit
> that never completes. The stuck thread prevents the framework from stopping
> any further bundles, so the JVM only exits when forcibly killed by the
> surrounding process manager. During that window the instance is half-dead but
> still visible to the rest of the cluster, delaying topology convergence on
> the surviving members.
> h3. Stack trace
> {noformat}
> FelixStartLevel
> at jdk.internal.misc.Unsafe.park([email protected]/Native Method)
> - parking to wait for <0x00000007d79a3e78> (a
> java.util.concurrent.CountDownLatch$Sync)
> at
> java.util.concurrent.locks.LockSupport.park([email protected]/LockSupport.java:221)
> at
> java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire([email protected]/AbstractQueuedSynchronizer.java:754)
> at
> java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly([email protected]/AbstractQueuedSynchronizer.java:1099)
> at
> java.util.concurrent.CountDownLatch.await([email protected]/CountDownLatch.java:230)
> at
> org.apache.jackrabbit.oak.plugins.document.CommitQueue$Entry.await(CommitQueue.java:346)
> at
> org.apache.jackrabbit.oak.plugins.document.CommitQueue.waitUntilHeadOfQueue(CommitQueue.java:285)}
> ...
> at
> com.adobe.granite.repository.impl.CRX3SessionImpl.save(CRX3SessionImpl.java:232)
> at
> org.apache.sling.jcr.resource.internal.helper.jcr.JcrResourceProvider.commit(JcrResourceProvider.java:617)
> at
> org.apache.sling.resourceresolver.impl.providers.stateful.AuthenticatedResourceProvider.commit(AuthenticatedResourceProvider.java:268)
> at
> org.apache.sling.resourceresolver.impl.helper.ResourceResolverControl.commit(ResourceResolverControl.java:565)
> at
> org.apache.sling.resourceresolver.impl.ResourceResolverImpl.commit(ResourceResolverImpl.java:1056)
> at
> org.apache.sling.discovery.oak.OakDiscoveryService.doUpdateProperties(OakDiscoveryService.java:550)
> at
> org.apache.sling.discovery.oak.OakDiscoveryService.bindPropertyProviderInteral(OakDiscoveryService.java:416)
> at
> org.apache.sling.discovery.oak.OakDiscoveryService.updatedPropertyProvider(OakDiscoveryService.java:430)
> - locked <0x00000006afc353a0> (a java.lang.Object)
> ... {noformat}
> h3. Root cause
> {{OakDiscoveryService}} declares {{PropertyProvider}} as a {{{}MULTIPLE{}}},
> {{DYNAMIC}} reference. Every bind / unbind / update callback triggers a JCR
> write via {{doUpdateProperties()}} → {{ResourceResolver.commit()}} in order
> to keep the properties node at {{//properties}} in sync with the currently
> registered providers.
> At shutdown, application bundles (higher start levels) typically stop before
> infrastructure bundles like {{discovery.oak}} (low start level). When an
> application bundle stops, its {{PropertyProvider}} services are unregistered,
> and Declarative Services invokes {{unbindPropertyProvider}} /
> {{updatedPropertyProvider}} on {{OakDiscoveryService}} synchronously on the
> Felix start-level thread. That callback then issues an Oak commit.
> If the Oak {{CommitQueue}} is slow at that moment — a pending entry ahead of
> us, repository under load, backlog on the cluster leader — the commit parks
> on a {{CountDownLatch}} and does not return. The Felix start-level thread is
> now stuck inside a callback for a bundle it hasn't finished stopping. It
> cannot advance to stop any further bundles, and in particular it cannot reach
> {{{}discovery.oak{}}}'s own bundle to trigger
> {{{}OakDiscoveryService.deactivate(){}}}. Shutdown stalls entirely.
> h3. Why the existing {{activated}} guard doesn't help
> {{OakDiscoveryService.activated}} is set to {{false}} inside
> {{{}deactivate(){}}}, several lines into the method, under
> {{{}viewStateManagerLock{}}}. In the scenario above {{deactivate()}} is never
> reached, so {{activated}} remains {{true}} and every unbind callback proceeds
> into {{doUpdateProperties()}} and into {{{}commit(){}}}.
> Even in cases where {{deactivate()}} does run, the {{activated}} flag is
> written under a different lock ({{{}viewStateManagerLock{}}}) from the one
> guarding bind/unbind ({{{}lock{}}}), so the two are not mutually observable
> without racing.
> h3. Why the property update is unnecessary at shutdown
> Once this instance is shutting down, rewriting the properties node has no
> value — no other cluster member will meaningfully act on the updated contents
> before this instance is removed from the topology anyway. The write is pure
> overhead, and during shutdown it is actively harmful because it holds the
> framework thread.
> h2. Impact
> Framework shutdown can hang for the full duration of the surrounding process
> manager's grace period before the JVM is forcibly killed.
> The stalled instance remains visible in the discovery topology throughout,
> delaying leader re-election and topology convergence for the rest of the
> cluster.
> Any operational tooling that assumes clean JVM exit (health checks, rolling
> restarts, orchestrated deployments) is affected.
> h2. Steps to reproduce
> Start a Sling instance with {{discovery.oak}} active and at least one
> {{PropertyProvider}} implementation registered by a bundle at a higher start
> level than {{{}discovery.oak{}}}.
> Induce Oak {{CommitQueue}} slowness — for example by holding an open write
> session on another thread, or by loading the repository with concurrent
> writers that outpace the commit rate.
> Trigger a clean framework shutdown (e.g. {{bin/stop}} or SIGTERM).
> Observe: shutdown does not complete. A thread dump shows {{FelixStartLevel}}
> parked in {{CommitQueue$Entry.await}} under {{{}doUpdateProperties{}}}. The
> JVM only exits on forcible termination.
> h2. Expected behaviour
> Once the framework has begun shutting down, {{OakDiscoveryService}} should
> skip all property-update writes and return immediately from
> {{{}doUpdateProperties(){}}}. Framework shutdown should proceed at its normal
> pace regardless of the state of the Oak {{{}CommitQueue{}}}.
> h2. Proposed fix (summary)
> Introduce a {{volatile boolean deactivating}} flag that short-circuits
> {{doUpdateProperties()}} once shutdown has begun. The flag must be set before
> the first blocking commit can start, which means it cannot rely solely on
> {{@Deactivate}} being invoked — because in the observed scenario
> {{deactivate()}} is never reached. The trigger should be one of:
> A {{FrameworkListener}} on {{{}FrameworkEvent.STOPPING{}}}, registered in
> {{{}@Activate{}}}, that sets the flag as soon as the framework begins
> shutting down. or
> An inline check of {{bundleContext.getBundle(0).getState() >=
> Bundle.STOPPING}} at the top of {{{}doUpdateProperties(){}}}.
> The flag is also set at the first line of {{deactivate()}} as a
> belt-and-braces measure for the case where {{discovery.oak}} is deactivated
> independently of a full framework stop.
> The fix must not interrupt an already-in-progress commit — that risks
> corrupting the Oak session. It only prevents new commits from starting once
> shutdown has begun.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
