sarutak commented on code in PR #57954:
URL: https://github.com/apache/spark/pull/57954#discussion_r3816651674
##########
core/src/main/java/org/apache/spark/security/CredentialProviderLoader.java:
##########
@@ -256,25 +261,23 @@ public static Set<String> discoverAllSchemes() {
* <p>
* This method iterates over all providers that have been initialized via
* {@link CredentialProvider#init(Map)} and calls {@link
CredentialProvider#close()}
- * on each. If any provider's {@code close()} throws, the exception is
suppressed
- * and attached to the first exception encountered. If at least one
exception occurred,
- * it is thrown after all providers have been attempted.
+ * on each. The first exception is retained, later exceptions are suppressed
onto it, and the
+ * first exception is rethrown after all providers have been attempted.
* <p>
- * After this method returns (normally or exceptionally), the initialization
tracking
- * is cleared, but the cached provider list is retained. This means
providers would be
- * re-initialized on the next {@link #providerFor} call (which is not
expected after
- * shutdown).
+ * After shutdown begins, subsequent {@link #providerFor} calls fail rather
than
+ * re-initializing a cached provider whose resources have already been
released.
* <p>
* <b>Contract:</b> {@code close()} implementations must not call back into
* {@code CredentialProviderLoader} methods (e.g., {@code providerFor}).
*
* @throws Exception if one or more providers threw during close
*/
- public static void closeAll() throws Exception {
+ public void closeAll() throws Exception {
List<CredentialProvider> toClose;
- synchronized (CredentialProviderLoader.class) {
+ synchronized (this) {
// Copy and clear under the lock to prevent double-close if closeAll()
is called
// again concurrently, and to avoid ConcurrentModificationException.
+ providersClosed = true;
Review Comment:
Thanks for pushing the shutdown wait. Bounding `stop()` to wait for the
in-flight renewal before closing providers is a good improvement, and I'd like
to keep it regardless of how we resolve the static-vs-instance question.
I think it's worth separating two things:
1. **The shutdown-wait** operates on `renewalExecutor`, an instance member
of `UserCredentialManager`, not of `CredentialProviderLoader`. It applies
identically to a static loader:
```scala
renewalExecutor.shutdownNow()
if (!renewalExecutor.awaitTermination(10, TimeUnit.SECONDS)) {
logWarning(log"Timed out waiting for the credential renewal task to
stop")
}
CredentialProviderLoader.closeAll() // static, with cachedProviders =
null
```
2. **The residual timeout window**: you're right that `awaitTermination` can
expire and let a straggling renewal thread call `providerFor()` after
`closeAll()`. But the mechanism that closes that window is the
`providersClosed` guard (throwing `IllegalStateException`), and that guard is
identical in both designs. It's exactly the point we agreed on earlier.
Instance scoping doesn't close this window any differently; a straggler holds a
reference to the instance loader just as it would reach the static one.
On the "reset requires coordination" concern: that only applies if
`providersClosed` is a *persistent* flag that must be flipped back for the next
manager. With `cachedProviders = null` in `closeAll()`, the null cache itself
is the "rediscover on next use" signal. The next manager's `getProviders()`
sees null and rediscovers fresh instances, no flag reset and no handshake with
the retiring manager. This is memory-safe for the same reason the existing DCL
is: `cachedProviders` is volatile and re-checked under the lock. So
`cachedProviders = null` covers rediscovery, and `providersClosed` covers
no-reopen-after-close. Both work statically without cross-manager coordination,
given the single-active-SparkContext lifecycle (manager A fully stops,
including this wait, before manager B starts).
So I'd frame the crux as a single question: is there a case where the
`providersClosed` guard is sufficient in the instance design but *not* in the
static one? If there is such an asymmetry (or a reproducible failure), I'd like
to understand it. That would settle the necessity for me. Otherwise the two
designs look equivalent on safety, and I'd prefer keeping the loader static and
landing the shutdown-wait on its own.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
