sarutak commented on code in PR #57954:
URL: https://github.com/apache/spark/pull/57954#discussion_r3772938645
##########
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. On the two concerns:
1. **Reset point for `providersClosed`**: We can simplify by dropping the
flag entirely. Setting `cachedProviders = null` in `closeAll()` is sufficient.
The null cache naturally means "rediscover on next use." No separate
closed/open state is needed.
2. **Lifecycle races between managers**: Spark enforces a single active
SparkContext per JVM via
[SparkContext.assertNoOtherContextIsRunning](https://github.com/apache/spark/blob/a2649b96d3a9a8af667f9b82c46359476a023373/core/src/main/scala/org/apache/spark/SparkContext.scala#L3069).
Two `UserCredentialManager` instances cannot coexist. The lifecycle is
strictly sequential (manager A stops, then manager B starts). There is no
window for one manager to invalidate providers used by another.
I intentionally chose a JVM-scoped loader because classpath-based
ServiceLoader discovery is a JVM-level invariant (the discovered providers
don't change between SparkContexts). Additionally, if we extend this to Spark
Connect multi-session in the future, a shared server-level loader would be more
appropriate. `CredentialProvider.resolve()` takes `UserContext` as an argument,
so session isolation is achieved at the call site without per-session loaders.
Instance-per-manager would mean redundant ServiceLoader discovery and separate
HTTP connection pools per session.
--
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]
