HyukjinKwon commented on code in PR #59196:
URL: https://github.com/apache/spark/pull/59196#discussion_r4230814268
##########
common/utils/src/main/scala/org/apache/spark/util/MavenUtils.scala:
##########
@@ -62,32 +65,126 @@ private[spark] object MavenUtils extends Logging {
}
}
- private def acquireIvyLock(isCancelled: () => Boolean): Unit = {
- def checkCancelled(): Unit = {
- if (isCancelled() || Thread.currentThread().isInterrupted) {
- throw new CancellationException("Maven dependency resolution was
cancelled")
+ private class IvyResolutionCancellation {
+ private val requested = new AtomicBoolean(false)
+ private val httpHandler = new AtomicReference[HttpClientHandler]()
+
+ def isCancelled: Boolean = requested.get()
+
+ def installUrlHandler(): () => Unit = {
+ val previousHandler = URLHandlerRegistry.getDefault
+ val handler = new HttpClientHandler
Review Comment:
**Non-blocking (P2):** `installUrlHandler` routes http/https through a new
stock Ivy `HttpClientHandler` for every cancellable resolution, even when
cancellation is never requested. That covers every
`RuntimeDependencyResolver.resolve` call: classic
`SparkSession.addArtifact("ivy://...")` and Spark Connect AddArtifacts Maven
dependencies. Before this change, these resolutions used Ivy's default
`BasicURLHandler` (JDK `HttpURLConnection` plus `IvyAuthenticator`), which
`spark-submit --packages` still uses.
In Ivy 2.5.3 and HttpClient 4.5.14 the two transports differ in ways that
matter for real deployments:
- `HttpClientHandler` builds its client with `HttpClients.custom()` and an
explicit `PoolingHttpClientConnectionManager()`, without
`useSystemProperties()`. Its TLS context therefore comes from
`SSLContexts.createDefault()` with no key managers, so client certificates from
`javax.net.ssl.keyStore` are never presented.
- Authentication is enabled only when Ivy's `CredentialsStore` has
credentials for the repository host, and the credentials provider reads only
`CredentialsStore`. It ignores `http.proxyUser`/`http.proxyPassword` and a
JVM-wide `java.net.Authenticator`, which `IvyAuthenticator` honors.
- Only the Digest, Basic and NTLM schemes are registered, so
Negotiate/Kerberos proxies stop working. A custom default `URLHandler`
registered by the application is also bypassed for http/https.
A driver behind an authenticated proxy configured through JVM properties, or
a repository that needs a client certificate, can still resolve with
`spark-submit --packages` but now fails with an unresolved-dependency error
through `addArtifact(ivy://...)` or Connect AddArtifacts. The PR describes
prompt cancellation as the only user-facing change, and the new tests use only
unauthenticated plain-HTTP and file repositories, so nothing catches this
regression.
**Recommended change:** Replace the stock Ivy HttpClientHandler in
IvyResolutionCancellation with a Spark-owned, per-resolution closeable Ivy URL
handler whose Apache HttpClient is built from the JVM's network configuration.
Use HttpClients.custom().useSystemProperties(), with no explicit connection
manager that would discard the system SSL socket factory, so it gets
SSLContext.getDefault() with keystore and truststore properties,
https.protocols/cipherSuites, system proxy routing, and the default
SPNEGO/Kerberos schemes. Give it a credentials provider that consults Ivy's
CredentialsStore first and then the JVM (java.net.Authenticator and
http(s).proxyUser/proxyPassword through SystemDefaultCredentialsProvider), with
authentication always enabled. Take over http/https only when the current
default is Ivy's stock BasicURLHandler, so a caller-installed custom default is
not bypassed. Keep the existing install, restore and close-on-cancel lifecycle
unchanged. Add runtime resolver tes
ts that cover JVM-level proxy authentication.
**Why this works:** Cancellation still aborts blocked reads by closing the
per-resolution connection pool. Because the client now draws proxy credentials,
Authenticator responses and TLS key material from the same JVM inputs that
HttpURLConnection/IvyAuthenticator use, a resolution that is never cancelled
authenticates and negotiates TLS the way spark-submit --packages does.
**Scope:** Make the per-resolution cancellable HTTP handler honor the JVM's
network configuration, so runtime Maven resolution keeps spark-submit's auth
and TLS behavior while staying promptly cancellable. This may add a new
Spark-owned handler class under common/utils' util package.
**Compatibility:** Prompt abort of a resolution blocked on an HTTP read, and
the cooperative cancel-then-join lifecycle that releases ivyLock and restores
the previous URL handler before CancellationException reaches the caller.
**Risks:** HttpClient's SPNEGO and the JDK's Negotiate handle JAAS subjects
differently, so Kerberos-authenticated proxies need explicit verification
rather than assumed parity. A process-wide
HttpsURLConnection.setDefaultSSLSocketFactory customization is still not
honored by HttpClient; only SSLContext.getDefault() and javax.net.ssl.* inputs
are.
**Constraints:** Must not call Thread.stop directly or indirectly; keep the
cooperative cancel-and-join lifecycle. Must restore the previous
URLHandlerRegistry default and close the per-resolution client on every exit
path. The non-cancellable resolveMavenCoordinates path must keep Ivy's existing
default handler.
**Success:** With JVM proxy credentials (http(s).proxyUser/proxyPassword) or
a default java.net.Authenticator, a runtime resolution through
RuntimeDependencyResolver authenticates to the proxy or repository the same way
spark-submit --packages does. With a client certificate configured through
javax.net.ssl.keyStore, the runtime path presents it during the TLS handshake.
Repositories authenticated through ivysettings <credentials> and
unauthenticated repositories still resolve on the runtime path. Cancelling a
resolution blocked on an HTTP read still raises CancellationException promptly,
only after the worker has exited, with ivyLock released and the previous URL
handler restored. A custom default URLHandler installed before resolution is
not bypassed for http/https.
**Verification:**
- **Regression:** A runtime resolution through an HTTP proxy that requires
Basic proxy authentication, with credentials supplied only through JVM proxy
credential properties or a default java.net.Authenticator, succeeds where the
stock HttpClientHandler path receives 407.
- **Behavior:** The blocked-read cancellation and uncooperative-worker
scenarios still produce a prompt CancellationException after the worker exits,
with the lock released and the global handler restored.
- **Compatibility:** Repositories authenticated through ivysettings
<credentials>, and unauthenticated repositories, resolve identically through
the runtime path and the non-cancellable spark-submit path.
- **Inspection:** The per-resolution client is built from JVM system
properties (system-default SSL context, system credentials provider), and the
handler is installed only over Ivy's stock default handler.
##########
common/utils/src/test/scala/org/apache/spark/util/MavenUtilsSuite.scala:
##########
@@ -308,6 +323,135 @@ class MavenUtilsSuite
}
}
+ test("runtime dependency resolver cancels active Ivy resolution") {
+ val executor = Executors.newFixedThreadPool(2)
+ implicit val executionContext: ExecutionContext =
+ ExecutionContext.fromExecutorService(executor)
+ val server = new ServerSocket(0, 1, InetAddress.getByName("127.0.0.1"))
+ val requestAccepted = new CountDownLatch(1)
+ val releaseRequest = new CountDownLatch(1)
+ val serverFuture = Future {
+ val socket = server.accept()
+ try {
+ requestAccepted.countDown()
+ releaseRequest.await()
+ } finally {
+ socket.close()
+ }
+ }
+ try {
+ val settings = Paths.get(tempIvyPath, "cancellable-ivysettings.xml")
+ Files.writeString(
+ settings,
+ s"""<ivysettings>
+ | <settings defaultResolver="blocked"/>
+ | <resolvers>
+ | <ibiblio name="blocked" m2compatible="true"
+ | root="http://127.0.0.1:${server.getLocalPort}"/>
+ | </resolvers>
+ |</ivysettings>""".stripMargin)
+ val cancelled = new AtomicBoolean(false)
+ val resolver = new RuntimeDependencyResolver(
+ ivySettingsPath = Some(settings.toString),
+ ivyPath = Some(Paths.get(tempIvyPath, "cancelled").toString))
+ val previousUrlHandler = URLHandlerRegistry.getDefault
+ val resolution = Future {
+ resolver.resolve(
+ URI.create("ivy://my.runtime.active:mylib:0.1"),
+ resolverTimeoutMs,
+ resolverTimeoutMs,
+ isCancelled = () => cancelled.get())
+ }
+
+ assert(requestAccepted.await(5, TimeUnit.SECONDS))
+ cancelled.set(true)
+ intercept[CancellationException] {
+ SparkThreadUtils.awaitResultNoSparkExceptionConversion(resolution,
5.seconds)
+ }
+ assert(URLHandlerRegistry.getDefault eq previousUrlHandler)
+
+ val next = MavenCoordinate("my.runtime.after.cancel", "mylib", "0.1")
+ IvyTestUtils.withRepository(next, None, None) { repo =>
+ val resolved = MavenUtils.resolveMavenCoordinates(
+ next.toString,
+ MavenUtils.buildIvySettings(Some(repo), Some(tempIvyPath)),
+ transitive = true,
+ isTest = true)
+ assert(resolved.nonEmpty)
+ }
+ } finally {
+ releaseRequest.countDown()
+ server.close()
+ SparkThreadUtils.awaitResultNoSparkExceptionConversion(serverFuture,
5.seconds)
Review Comment:
**Nit (P3):** Suppose the resolution never reaches the fake server, for
example because it fails before connecting. Then `requestAccepted.await`
returns false and the assertion throws. `server.close()` makes the
still-pending `accept()` throw, and this
`awaitResultNoSparkExceptionConversion` rethrows that `SocketException` out of
the `finally`. That replaces the real assertion failure and skips
`executor.shutdownNow()`. A still-running resolution is then never interrupted
and can hold `ivyLock` until its 30 s network timeout, stalling or failing
later tests in the suite and making the original failure hard to diagnose.
**Verification:**
- **Regression:** When the blocked-read test's connection precondition
fails, the original assertion failure is reported and no executor thread or
held ivyLock remains.
- **Behavior:** On the normal path the test still proves prompt
cancellation, handler restoration and lock release.
--
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]
