dongjoon-hyun commented on code in PR #57677:
URL: https://github.com/apache/spark/pull/57677#discussion_r3691639936
##########
core/src/main/java/org/apache/spark/security/CredentialProviderLoader.java:
##########
@@ -251,6 +251,51 @@ public static Set<String> discoverAllSchemes() {
return schemes;
}
+ /**
+ * Closes all initialized providers, suppressing individual close exceptions.
+ * <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.
+ * <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).
+ * <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 {
+ synchronized (CredentialProviderLoader.class) {
+ // Copy and clear first to prevent double-close if closeAll() is called
again
+ // concurrently or re-entrantly, and to avoid
ConcurrentModificationException
+ // if a close() implementation were to interact with this class.
+ List<CredentialProvider> toClose = new ArrayList<>(initializedProviders);
+ initializedProviders.clear();
+
+ Exception firstException = null;
+ for (CredentialProvider provider : toClose) {
+ try {
+ provider.close();
+ } catch (Exception e) {
+ if (firstException == null) {
+ firstException = e;
+ } else {
+ firstException.addSuppressed(e);
+ }
+ }
+ }
+ if (firstException != null) {
+ throw firstException;
+ }
+ }
+ }
Review Comment:
`closeAll()` invokes third-party `close()` implementations while holding the
class lock. Since `providerFor()` takes the same lock on every call, a
`close()` that blocks (e.g., draining an HTTP connection pool) stalls all
credential resolutions — and if a `close()` implementation waits on a thread
that is calling `providerFor()`, we get a deadlock. The javadoc contract ("must
not call back into `CredentialProviderLoader`") only covers direct re-entry,
not indirect waiting.
Since the copy-and-clear already happens under the lock, moving just the
close loop outside the lock removes the risk:
```suggestion
public static void closeAll() throws Exception {
List<CredentialProvider> toClose;
synchronized (CredentialProviderLoader.class) {
// Copy and clear first to prevent double-close if closeAll() is
called again,
// and to avoid ConcurrentModificationException if a close()
implementation
// were to interact with this class.
toClose = new ArrayList<>(initializedProviders);
initializedProviders.clear();
}
// Close outside the lock so a slow or blocking close() cannot stall
// providerFor() callers or deadlock against them.
Exception firstException = null;
for (CredentialProvider provider : toClose) {
try {
provider.close();
} catch (Exception e) {
if (firstException == null) {
firstException = e;
} else {
firstException.addSuppressed(e);
}
}
}
if (firstException != null) {
throw firstException;
}
}
```
--
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]
