xDS: fix DistributorWatcher thread safety in CertificateProvider

[bcleenders]

A gRPC client using xDS-driven mTLS does two things concurrently:

1. Certificate provider: watches cert files on disk, periodically reloads them, and notifies subscribers when certs change.
2. Channel subscriptions: each gRPC channel that needs mTLS registers a watcher on the cert provider to get the TLS credentials.

The DistributorWatcher sits 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:

1. updateSpiffeTrustMap isn't synchronized (unlike updateCertificate and updateTrustedRoots), so it can race against addWatcher/removeWatcher from another thread.
2. if callbacks mutate their own subscription. A callchain (line numbers per 1.83.0 / 1084e97) for that is:

  DistributorWatcher.onError()                              CertificateProvider.java:126
    → for (watcher : downstreamWatchers)                    CertificateProvider.java:127
      → watcher.onError(errorStatus)                        CertificateProvider.java:128
        → DynamicSslContextProvider.onError()               DynamicSslContextProvider.java:143
          → callback.onException(error)                     DynamicSslContextProvider.java:145
            → SslContextProviderSupplier.onException()      SslContextProviderSupplier.java:79
              → releaseSslContextProvider(toRelease)        SslContextProviderSupplier.java:81
                → TlsContextManagerImpl.releaseClient...()  TlsContextManagerImpl.java:78
                  → ReferenceCountingMap.releaseInternal()  ReferenceCountingMap.java:89
                    → if refcount == 0: value.close()       ReferenceCountingMap.java:95
                      → CertProviderSslContextProvider.close()  CertProviderSslContextProvider.java:216
                        → certHandle.close()                CertProviderSslContextProvider.java:218
                          → Handle.close()                  CertificateProviderStore.java:62
                            → distWatcher.removeWatcher()   CertificateProviderStore.java:64  <-- REENTRANT

We observed ConcurrentModificationException in updateSpiffeTrustMap() 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:

DEADLINE_EXCEEDED: CallOptions deadline exceeded after 4.998s.
Name resolution delay 0.000000000 seconds.
[closed=[], open=[[connecting_and_lb_delay=4999ms, was_still_waiting]]]

  io.grpc.StatusException: UNKNOWN
    at io.grpc.xds.internal.security.DynamicSslContextProvider.onError(DynamicSslContextProvider.java:145)
    at io.grpc.xds.internal.security.certprovider.CertificateProvider$DistributorWatcher.onError(CertificateProvider.java:128)
    at io.grpc.xds.internal.security.certprovider.FileWatcherCertificateProvider.checkAndReloadCertificates(FileWatcherCertificateProvider.java:149)
  Caused by: java.util.ConcurrentModificationException
    at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1606)
    at java.base/java.util.HashMap$KeyIterator.next(HashMap.java:1629)
    at io.grpc.xds.internal.security.certprovider.CertificateProvider$DistributorWatcher.updateSpiffeTrustMap(CertificateProvider.java:120)
    at io.grpc.xds.internal.security.certprovider.FileWatcherCertificateProvider.checkAndReloadCertificates(FileWatcherCertificateProvider.java:144)

java.util.ConcurrentModificationException
  at java.util.HashMap$HashIterator.nextNode(HashMap.java:1606)
  at java.util.HashMap$KeyIterator.next(HashMap.java:1629)
  at CertificateProvider$DistributorWatcher.onError(CertificateProvider.java:127)
  at FileWatcherCertificateProvider.checkAndReloadCertificates(FileWatcherCertificateProvider.java:149)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bcleenders / name: bcleenders (0713c23, f273d15)

[ejona86]

My drive-by thoughts for @kannanjgithub,

• We could consider swapping to CopyOnWriteArraySet, but I don't know is the step in the right direction
• We should add @GuardedBy to prevent simple unguarded accesses
• Ideally we wouldn't call arbitrary code with the lock held. That prevents deadlocks and reentrancy (which is the cause of the concurrent modification here). You can't just drop the lock before calling the watcher though, as then you have concurrent callbacks from things like addWatcher() vs updateTrustedRoots(). We typically solve that by using a SynchronizationContext, enqueuing while the lock is held, and then draining it after dropping the lock. That would be an alternative to CopyOnWriteArraySet

[bcleenders]

Thanks for the comment!

I restructured it to follow the pattern you suggested; 1) GuardedBy to help with static checking, 2) using SyncContext to separate scheduling the callbacks vs. executing the callbacks.

There is a change in behavior; exceptions thrown in callbacks are no longer propagated back. The only code (afaict) using it was FileWatcherCertificateProvider, which would skip updating lastModifiedTime if an exception was thrown, so the next refresh cycle would retry the update. But since the retry reloads the same file contents, a deterministic callback failure (e.g. certificate validation error) now repeats the same error until the file changes on disk, so I think it's not a meaningful change (but this is all very new to me, so please correct me if I'm wrong!).

The SyncContext approach also fixes the error that I had aimed to fix in #13089 , so I'll close that PR.