sarutak commented on code in PR #57954:
URL: https://github.com/apache/spark/pull/57954#discussion_r3824026383
##########
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 @cloud-fan. I'm satisfied that the `awaitTermination` timeout
scenario presents a genuine asymmetry: after the timeout, a straggler thread
from the retiring manager can still be alive when a new SparkContext creates a
new manager, and a static boolean flag cannot distinguish the straggler from
the new manager's legitimate calls. Instance scoping provides lifecycle
isolation by construction without additional coordination mechanisms. LGTM
overall.
Two requests before merge:
**1. Please update the PR description** to reflect the actual motivation
established in our discussion. The current description mentions "rediscover and
reinitialize the released provider instances," which is the stale-cache problem
solvable by `cachedProviders = null`. The real justification is the
`awaitTermination` timeout scenario: after the timeout expires, the straggler
from the old manager could pass through a reset flag and interact with the new
manager's provider state. Instance scoping provides identity separation that a
shared boolean cannot. Future readers should understand why `cachedProviders =
null` alone was insufficient.
**2. `discoverAllSchemes()` should check `providersClosed`**.
`providerFor()` guards against use after close, but `discoverAllSchemes()` does
not. Since this class loads arbitrary third-party providers via ServiceLoader,
we cannot assume all implementations keep `supportedSchemes()` safe to call
after `close()`. A misbehaving provider could throw an exception that aborts
the loop, preventing scheme discovery for other providers as well. Adding the
same guard as `providerFor()` would make the API consistent and defensive:
```java
public Set<String> discoverAllSchemes() {
synchronized (this) {
if (providersClosed) {
throw new IllegalStateException("Credential providers have already
been closed");
}
}
List<CredentialProvider> providers = getProviders();
// ...
}
```
--
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]
