Repository navigation
xDS: fix DistributorWatcher thread safety in CertificateProvider - #13087
bcleenders wants to merge 4 commits into
Conversation
`DistributorWatcher.updateSpiffeTrustMap()` is not `synchronized`, unlike `updateCertificate()` and `updateTrustedRoots()` in the same class. This causes `ConcurrentModificationException` when a watcher is added or removed while `updateSpiffeTrustMap` iterates `downstreamWatchers`. Additionally, all update methods iterate `downstreamWatchers` directly. If a callback reentrantly calls `addWatcher`/`removeWatcher`, the `HashSet` is modified during iteration, producing: ``` java.util.ConcurrentModificationException at java.util.HashMap$HashIterator.nextNode(HashMap.java:1606) at java.util.HashMap$KeyIterator.next(HashMap.java:1629) at DistributorWatcher.updateSpiffeTrustMap(CertificateProvider.java:120) ``` Observed in production when a service creates many mTLS-enabled gRPC channels concurrently at startup (~60 channels across 7 threads). All channels share the same `file_watcher` cert provider instance. The burst of concurrent `addWatcher()` calls races with the provider's initial `updateSpiffeTrustMap()` call, triggering the CME. The exception propagates to `DynamicSslContextProvider.onError()`, preventing the TLS context from being created. Affected subchannels remain in CONNECTING until the deadline expires: ``` DEADLINE_EXCEEDED: CallOptions deadline exceeded after 4.998s. Name resolution delay 0.000000000 seconds. [closed=[], open=[[connecting_and_lb_delay=4999ms, was_still_waiting]]] ``` These tests demonstrate the race. They are expected to fail until the fix is applied.
|
My drive-by thoughts for @kannanjgithub,
|
Add `synchronized` to `updateSpiffeTrustMap()`, matching `updateCertificate()` and `updateTrustedRoots()`. Iterate a snapshot (`ImmutableSet.copyOf`) of `downstreamWatchers` in all notification methods, instead of iterating the live `HashSet`. This prevents `ConcurrentModificationException` when a callback calls `addWatcher()`/`removeWatcher()`. Mark other functions (`getLastIdentityCert()`, `close()` & `clearValues()`) which read/write state without holding the monitor as synchronized.
409796b to
70e13fa
Compare
|
Thanks for the comment! I restructured it to follow the pattern you suggested; 1) There is a change in behavior; exceptions thrown in callbacks are no longer propagated back. The only code (afaict) using it was The SyncContext approach also fixes the error that I had aimed to fix in #13089 , so I'll close that PR. |
Reviewer feedback: instead of holding the intrinsic lock during watcher callbacks (and using `ImmutableSet.copyOf` to tolerate reentrant modification), decouple state mutation from callback delivery using `SynchronizationContext`. Under the intrinsic lock, state is updated and callbacks are enqueued via `executeLater`. After releasing the lock, `drain()` delivers the callbacks. This guarantees: - No lock held during callbacks (prevents deadlocks with downstream locks such as `ReferenceCountingMap`) - Serialized, non-reentrant delivery (a callback that calls `addWatcher` enqueues work that runs after the current callback) This also subsumes the `DynamicSslContextProvider.onError` deadlock fix from grpc#13089: the `DistributorWatcher` lock is no longer held when `onError` callbacks run, so the lock-ordering cycle between `DistributorWatcher` and `ReferenceCountingMap` cannot occur. Exceptions from watcher callbacks no longer propagate to the caller (`SynchronizationContext` catches and logs them). The only caller that relied on propagation is `FileWatcherCertificateProvider`, which skips a `lastModifiedTime` update when `updateTrustedRoots` throws, causing a retry on the next refresh cycle. This retry was ineffective: the same cert data produces the same deterministic failure (e.g., a static `CertificateValidationContext` rejecting the cert), so retrying just repeats the same error until the file changes on disk. The error is still delivered to the affected watcher via `DynamicSslContextProvider.onError()`.
70e13fa to
fc04aa5
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a SynchronizationContext to CertificateProvider's DistributorWatcher to execute all watcher callbacks safely and prevent potential deadlocks. It also adds a comprehensive test suite in CertificateProviderTest.java to verify notification delivery and concurrency behaviors. The reviewer suggested an improvement in the onError method to eliminate an unnecessary ArrayList allocation, noting that iterating over downstreamWatchers directly within the synchronized block is safe because syncContext.executeLater only enqueues the callback.
| public void onError(Status errorStatus) { | ||
| List<Watcher> watchers; | ||
| synchronized (this) { | ||
| watchers = new ArrayList<>(downstreamWatchers); | ||
| } | ||
| for (Watcher watcher : watchers) { | ||
| syncContext.executeLater(() -> watcher.onError(errorStatus)); | ||
| } | ||
| syncContext.drain(); | ||
| } |
There was a problem hiding this comment.
In onError, copying downstreamWatchers to a new ArrayList and iterating outside the lock is unnecessary. Since syncContext.executeLater only enqueues the callback and does not execute any user code or block, it is safe to iterate over downstreamWatchers directly inside the synchronized(this) block. This avoids the overhead of allocating a new list and copying elements, and makes the implementation consistent with updateCertificate, updateTrustedRoots, and updateSpiffeTrustMap.
public void onError(Status errorStatus) {
synchronized (this) {
for (Watcher watcher : downstreamWatchers) {
syncContext.executeLater(() -> watcher.onError(errorStatus));
}
}
syncContext.drain();
}
|
/gcbrun |
kannanjgithub
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
This branch has not been deployed
4 participants
A gRPC client using xDS-driven mTLS does two things concurrently:
The
DistributorWatchersits in the middle and fans out: one cert provider pushes updates to it, and it distributes to N channel watchers.I think there are two conditions in this that can lead to a
ConcurrentModificationException:updateSpiffeTrustMapisn't synchronized (unlikeupdateCertificateandupdateTrustedRoots), so it can race againstaddWatcher/removeWatcherfrom another thread.We observed
ConcurrentModificationExceptioninupdateSpiffeTrustMap()in production, consistent with the unsynchronized iteration described above. We also observed startup RPC deadlines during the rollout, but have not established the causal connection between these symptoms.These all occurred in a service that creates many (~60) channels on startup, while getting the CDS config over xDS. The service uses grpc-java / grpc-xds
1.83.0. When we switched it to use mTLS with SPIFFE certificates, we got the following errors: