cloud-fan commented on code in PR #57954:
URL: https://github.com/apache/spark/pull/57954#discussion_r3819186340
##########
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. The asymmetry appears when the next manager starts after the bounded
shutdown wait expires.
With a static loader, manager A must leave `providersClosed = true` to
reject its straggling renewal task. Manager B must then reopen or reset that
same global loader. Once reopened, the loader cannot distinguish manager B from
the straggler from manager A, so both can discover or initialize the new
providers. Setting `cachedProviders = null` handles rediscovery, but it does
not provide lifecycle identity.
The single-active-`SparkContext` rule does not remove this window. After
`awaitTermination()` times out, `stop()` intentionally continues while the
renewal task from manager A may still be running, and a later context can start.
With instance scoping, the straggler from manager A retains the permanently
closed loader A and fails the `providersClosed` check, while manager B receives
a fresh loader B and proceeds normally. That is the safety difference; I agree
that the shutdown wait itself remains useful in both designs.
I pushed 9b0ee52a3d9 with a focused test for this lifecycle-generation case.
The test keeps a stale caller holding loader A, closes A, successfully
initializes loader B, and only then releases the stale caller and verifies that
it is still rejected by A. I also moved the closed-state check to the
`providerFor()` boundary so a retired loader consistently rejects calls even
when the requested scheme has no candidates, while retaining the later check
for a concurrent `closeAll()` during selection.
I also agree that the classloader point is secondary, and instance scoping
does not prevent a provider already inside `resolve()` from being closed after
the timeout. Its purpose here is specifically to prevent work from an old
lifecycle from entering the next lifecycle provider state.
--
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]
