seokmo-yoo-dev commented on PR #23393:
URL: https://github.com/apache/kafka/pull/23393#issuecomment-5568412628
Some detail that did not belong in the commit message.
The observed failure, from a Flink session cluster where each job gets its
own
class loader:
```
javax.security.auth.login.LoginException: java.lang.IllegalStateException:
Trying to access closed classloader
at javax.security.auth.login.LoginContext.invoke(LoginContext.java:691)
at javax.security.auth.login.LoginContext.login(LoginContext.java:587)
at
org.apache.kafka.common.security.oauthbearer.internals.expiring.ExpiringCredentialRefreshingLogin.reLogin(ExpiringCredentialRefreshingLogin.java:390)
```
The sequence:
1. Deployment A creates a client. Its `LoginManager` is cached and the
refresh
thread starts, inheriting A's context class loader because `KafkaThread`
does
not set one.
2. Deployment B creates a client with the same JAAS configuration, receives
A's
`LoginManager`, and raises the reference count to two.
3. A stops and releases its reference. The count drops to one, so
`login.close()` never runs and the thread keeps going with A's class
loader.
4. The container discards A's class loader. `LoginContext` captures the
context
class loader at construction and resolves login modules through
`ServiceLoader`, and `reLogin()` builds its `LoginContext` on that same
thread, so the class loader never changes and every retry fails
identically.
5. The credential expires and clients that need to re-authenticate are
rejected.
The last step lags the rest, since authentication happens only when a
connection
is established. Open connections keep working with an expired token, and
which
client breaks first depends on reconnection timing, which makes this hard to
trace back to the redeployment that caused it.
On why the key rather than somewhere else, and a question for reviewers: the
key
has been widened twice before, by KIP-86 for the login and callback classes
and
by KAFKA-14676 for the `sasl.*` configs, each time after a dimension that
must
not be shared turned out to be missing from it. The class loader looks like
the
same pattern to me, but KIP-83 says only that caching is "keyed on the jaas
configuration object" and does not mention class loaders, so I could not
settle
from the design documents whether sharing across class loaders was intended.
If
it was, the fix belongs elsewhere, most likely in having
`ExpiringCredentialRefreshingLogin` set the context class loader on its own
thread rather than inheriting one.
On retention, since the key now holds a class loader: the reference lives
only
as long as the cache entry, and `release()` removes the entry when the last
caller lets go. Before this change a leaked entry retained the class loader
anyway through the live refresh thread, which was the defect.
--
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]
