cloud-fan commented on code in PR #57954:
URL: https://github.com/apache/spark/pull/57954#discussion_r3796879305
##########
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 continuing the discussion. I agree with the core point that
instance scoping alone does not prevent a provider already in `resolve()` from
being closed concurrently. I pushed 84fae7afd9b to address that directly:
`stop()` now interrupts the renewal executor and waits up to 10 seconds for the
in-flight task to terminate before closing providers. The bound avoids hanging
Spark shutdown indefinitely; if it expires, we warn and continue. The test now
verifies that the provider remains open while an interrupt-resistant renewal is
still running and is closed only after that renewal exits.
I still don't think `cachedProviders = null` is sufficient as the overall
lifecycle fix. `shutdownNow()` requests interruption but does not establish
that the old renewal has finished. Without a closed guard, that task can resume
and rediscover or initialize providers after `closeAll()`. Adding a static
`providersClosed` guard prevents that, but the next manager then needs to reset
global state. Determining when that reset is safe requires coordination with
the retiring manager, which is precisely the lifecycle coupling this PR removes.
With the current design, the ownership rule is explicit: each manager owns
one loader; once stopped, that loader remains permanently closed; and a later
manager receives a fresh loader. The bounded executor wait protects provider
use within that lifecycle, while instance ownership prevents work from an old
lifecycle from mutating the next lifecycle's provider cache.
I also agree that both implementations consult the thread context
classloader. The relevant distinction is ownership: a static cache permanently
associates discovery with whichever context classloader initializes it first,
whereas a per-manager loader discovers under that manager's lifecycle. If
Connect later needs server-scoped provider pooling, that should be introduced
with explicit server-level ownership rather than inferred from JVM-global
static state.
Given the negligible cost of one ServiceLoader scan per sequential
SparkContext, I believe the instance-scoped design plus the shutdown wait is
the safer and clearer model.
--
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]
