Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up Jvm.* subprocess/inprocess APIs (500USD Bounty) #3772

Open
lihaoyi opened this issue Oct 19, 2024 · 2 comments · May be fixed by #3775 or #4456
Open

Clean up Jvm.* subprocess/inprocess APIs (500USD Bounty) #3772

lihaoyi opened this issue Oct 19, 2024 · 2 comments · May be fixed by #3775 or #4456
Labels
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Oct 19, 2024


From the maintainer Li Haoyi: I'm putting a 500USD bounty on this issue, payable by bank transfer on a merged PR implementing this.


The various Jvm.* methods are a mess. #3770 makes them marginally more messy, but they've been messy for years and could use a cleanup.

The goal of this issue is to consolidate all subprocess and classloader spawning operations in Jvm.scala into (approximately) the following four signatures

  def spawn(mainClass: String,
                      classPath: Iterable[os.Path],
                      jvmArgs: Seq[String],
                      mainArgs: Seq[String],
                      env: Map[String, String] = null,
                      cwd: Path = null,
                      stdin: ProcessInput = Pipe,
                      stdout: ProcessOutput = Pipe,
                      stderr: ProcessOutput = os.Inherit,
                      mergeErrIntoOut: Boolean = false,
                      propagateEnv: Boolean = true,
                      useCpPassingJar: Boolean = false): os.SubProcess = {

    val commandArgs = jvmCommandArgs(javaExe, mainClass, jvmArgs, classPath, mainArgs, useCpPassingJar)
    os.spawn(commandArgs, env, cwd, stdin, stdout, stderr,  mergeErrIntoOut,  propagateEnv)
  }

  def call(mainClass: String,
                     classPath: Iterable[os.Path],
                     jvmArgs: Seq[String],
                     mainArgs: Seq[String],
                     env: Map[String, String] = null,
                     cwd: Path = null,
                     stdin: ProcessInput = Pipe,
                     stdout: ProcessOutput = Pipe,
                     stderr: ProcessOutput = os.Inherit,
                     mergeErrIntoOut: Boolean = false,
                     timeout: Long = -1,
                     check: Boolean = true,
                     propagateEnv: Boolean = true,
                     timeoutGracePeriod: Long = 100,
                     useCpPassingJar: Boolean = false): os.CommandResult = {

    val commandArgs = jvmCommandArgs(javaExe, mainClass, jvmArgs, classPath, mainArgs, useCpPassingJar)

    os.call(
      commandArgs,
      env,
      cwd,
      stdin,
      stdout,
      stderr,
      mergeErrIntoOut,
      timeout,
      check,
      propagateEnv,
      timeoutGracePeriod
    )
  }

  def spawnClassloader(classPath: Iterable[os.Path],
                       sharedPrefixes: Seq[String],
                       isolated: Boolean = true): java.net.URLClassLoader = {
    mill.api.ClassLoader.create(
      classPath.iterator.map(_.toNIO.toUri.toURL).toVector,
      if (isolated) null else getClass.getClassLoader,
      sharedPrefixes = sharedPrefixes
    )()
  }

  def callClassloader[T](classPath: Iterable[os.Path],
                         sharedPrefixes: Seq[String],
                         isolated: Boolean = true)(f: ClassLoader => T): T = {
    val oldClassloader = Thread.currentThread().getContextClassLoader
    val newClassloader = spawnClassloader(classPath, sharedPrefixes, isolated)
    Thread.currentThread().setContextClassLoader(newClassloader)
    try {
      f(newClassloader)
    } finally {
      Thread.currentThread().setContextClassLoader(oldClassloader)
      newClassloader.close()
    }
  }


  private def jvmCommandArgs(javaExe: String,
                             mainClass: String,
                             jvmArgs: Seq[String],
                             classPath: Agg[os.Path],
                             mainArgs: Seq[String],
                             useCpPassingJar: Boolean): Vector[String] = {
    val classPath2 =
      if (useCpPassingJar && !classPath.iterator.isEmpty) {
        val passingJar = os.temp(prefix = "run-", suffix = ".jar", deleteOnExit = false)
        createClasspathPassingJar(passingJar, classPath)
        Agg(passingJar)
      } else classPath

    Vector(javaExe) ++
      jvmArgs ++
      Vector("-cp", classPath2.iterator.mkString(java.io.File.pathSeparator), mainClass) ++
      mainArgs
  }

Jvm.spawn and Jvm.call intentionally follow the signatures of os.spawn and os.call. Jvm.spawnClassloader and Jvm.callClassloader are in-memory variations of the theme, with no subprocess-related parameters and an API tweaked to work in-memory.

  1. All existing subprocess/classloader APIs in Jvm.scala should be refactored to go through the four APIs above. The other APIs should all be deprecated.
  2. All code outside Jvm.scala spawning subprocesses or classloaders should go through those four APIs,
  3. We should ensure all tests pass both with and without change (2) above to ensure that the behavior of the existing methods is maintained (at least as far as they are covered by Mill's existing test suite)
@lihaoyi lihaoyi added this to the 0.13.0 milestone Oct 19, 2024
@lihaoyi lihaoyi linked a pull request Oct 20, 2024 that will close this issue
@lihaoyi lihaoyi changed the title Clean up Jvm.* subprocess/inprocess APIs Clean up Jvm.* subprocess/inprocess APIs (500USD Bounty) Oct 24, 2024
@lihaoyi lihaoyi added the bounty label Oct 24, 2024
vishwamartur added a commit to vishwamartur/mill that referenced this issue Nov 16, 2024
Related to com-lihaoyi#3772

Refactor `Jvm.scala` to consolidate subprocess and classloader spawning operations into four specified signatures.

* **Refactor `callSubprocess` method:**
  - Rename to `call`.
  - Update parameters to match the specified `call` signature.
  - Use `jvmCommandArgs` to generate command arguments.
  - Call `os.call` with the updated parameters.

* **Refactor `runSubprocess` method:**
  - Rename to `spawn`.
  - Update parameters to match the specified `spawn` signature.
  - Use `jvmCommandArgs` to generate command arguments.
  - Call `os.spawn` with the updated parameters.

* **Add `spawnClassloader` method:**
  - Create a new method to match the specified `spawnClassloader` signature.
  - Use `mill.api.ClassLoader.create` to create a classloader.

* **Add `callClassloader` method:**
  - Create a new method to match the specified `callClassloader` signature.
  - Use `spawnClassloader` to create a classloader and set it as the context classloader.
  - Execute the provided function with the new classloader and restore the old classloader afterward.

* **Add tests in `JvmTests.scala`:**
  - Add tests for the new `call` method.
  - Add tests for the new `spawn` method.
  - Add tests for the new `callClassloader` method.
  - Add tests for the new `spawnClassloader` method.
lihaoyi added a commit that referenced this issue Dec 9, 2024
…4085)

Fixes #657

We re-use `MillBackgroundWrapper` and extend it to optionally spawn
subprocess instead of calling the in-process main method. There's a bit
of overhead in the JVM wrapper, but we need some kind of process wrapper
to manage the mutex and termination even when the Mill process
terminates, and eventually when
#4007 lands we can use that to
provide lighter-weight wrappers. We add a shutdown hook and copy the
termination grace-period logic from OS-Lib to try and gently kill the
wrapped process when necessary

Integrated this into `PythonModule#runBackground` and added a unit test
to verify the asynchronous launch, background existence (by checking
that a file lock is taken) and termination on deletion. We can integrate
this into `TypescriptModule` as well but punting that for later

The subprocess APIs are a mess but leaving that to fix in 0.13.0
#3772
@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 13, 2025

As part of this we should move def launcher from JavaModule.scala to RunModule.scala, which apparently cannot be done without breaking bincompat

jodersky pushed a commit to jodersky/mill that referenced this issue Jan 14, 2025
…om-lihaoyi#4085)

Fixes com-lihaoyi#657

We re-use `MillBackgroundWrapper` and extend it to optionally spawn
subprocess instead of calling the in-process main method. There's a bit
of overhead in the JVM wrapper, but we need some kind of process wrapper
to manage the mutex and termination even when the Mill process
terminates, and eventually when
com-lihaoyi#4007 lands we can use that to
provide lighter-weight wrappers. We add a shutdown hook and copy the
termination grace-period logic from OS-Lib to try and gently kill the
wrapped process when necessary

Integrated this into `PythonModule#runBackground` and added a unit test
to verify the asynchronous launch, background existence (by checking
that a file lock is taken) and termination on deletion. We can integrate
this into `TypescriptModule` as well but punting that for later

The subprocess APIs are a mess but leaving that to fix in 0.13.0
com-lihaoyi#3772
@He-Pin
Copy link
Contributor

He-Pin commented Jan 15, 2025

Is this still valid, I would like to work on it this weekend.

@sake92 sake92 linked a pull request Feb 2, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants