Mill clean should stop/prevent reuse of previously created workers.

[roman-mibex-2]

TLDR: mill clean should stop workers / or discard them.

I have a mill build with some webpack workers, which stay up. I sometimes had a puzzling state
where these workers seem complete out of sync. I've noticed that a ./mill clean breaks them.

Here is a small demostration, without webpack ;)

build.sc:

import mill._
import os.Path

object `mill-demo` extends RootModule  {
  def resources = T.source(millSourcePath/"barely-changing.txt")
  def source = T.source(millSourcePath/"frequently-changing.js")

  def webpackWorker = T.worker{
    // When the worker starts, we copy stuff from other targets
    // These targets rarely change, and we want to keep them 'hot' in the worker
    os.copy(resources().path,T.dest/"resource.txt",replaceExisting = true)
    new MyWebpackWorker(T.dest)
  }

  def webpack = T{
    webpackWorker().compile(source().path)
  }

  class MyWebpackWorker(myWorkingDir:Path){
    println("Worker is starting up")
      def compile(latest:Path)={
        os.copy(latest,myWorkingDir/"code.js", replaceExisting = true)
        val state = PathRef(myWorkingDir)
        println(s"State of working dir is ${state}")
        state
      }
  }
}

Demostration of the issue:

$ date > barely-changing.txt
$ date > frequently-changing.js

# Initial build, all fine
$ ./mill show webpack
[1/1] show > [2/4] webpackWorker 
Worker is starting up, worker:1930565333
[1/1] show > [4/4] webpack 
worker: 1930565333
"ref:v0:51849a16:/home/roman/dev/private-dev/mill-demo/out/webpackWorker.dest"

# I change the source code, all works as expected
$ date > frequently-changing.js
$ ./mill show webpack
[1/1] show > [4/4] webpack
worker: 1930565333
"ref:v0:800319cb:/home/roman/dev/private-dev/mill-demo/out/webpackWorker.dest"

# In the rare occasion the resources change, the worker is restarted, works as expected
$ ./mill show webpack
[1/1] show > [2/4] webpackWorker 
Worker is starting up, worker:2000339927
[1/1] show > [4/4] webpack 
worker: 2000339927
"ref:v0:9faaa5de:/home/roman/dev/private-dev/mill-demo/out/webpackWorker.dest"

# However, as soon as mill clean ran, hell breaks loose
# The worker is still alive, but the underlying workind directory was wiped
[1/1] show > [4/4] webpack 
worker: 2000339927
1 targets failed
show 1 targets failed
webpack java.nio.file.NoSuchFileException: /home/roman/dev/private-dev/mill-demo/out/webpackWorker.dest/code.js
    java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
    java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
    java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
    java.base/sun.nio.fs.UnixFileSystem.copyFile(UnixFileSystem.java:673)
    java.base/sun.nio.fs.UnixFileSystem.copy(UnixFileSystem.java:1084)
    java.base/sun.nio.fs.UnixFileSystemProvider.copy(UnixFileSystemProvider.java:300)
    java.base/java.nio.file.Files.copy(Files.java:1304)
    os.copy$.copyOne$1(FileOps.scala:200)
    os.copy$.apply(FileOps.scala:204)
    millbuild.build$mill$minusdemo$MyWebpackWorker.compile(build.sc:28)
    millbuild.build$mill$minusdemo$.$anonfun$webpack$3(build.sc:19)
roman@roman-lenovo24 ~/d/p/mill-demo (master) [1]> 

As demostrated, the mill clean cleans the out directory, but does not stop the worker.
Then, when the worker is invoked again, it's working directory is wiped.
In this demostration, it simply crashes.

In the real work this ends up with a webpack worker with a broken working directory,
producting garbage results.

Therefore, I think that mill clean should kill/restart workers.
Better solutions are of course also welcome.

[lefou]

Interesting. Stopping all workers can be costly, especially closing the ubiquitously used ZincWorker would lead to drastically increased build times after a clean. So, the current behavior isn't something we should change lightly. On the other hand, since workers have a T.dest, it should be safe to use it. So I agree, there should be a way to close a worker on clean or handle the clean event in some other way.

Maybe, we could create some analogy to T.persistent, and add a second worker type, which is T.dest aware?

As an alternative, we could introspect the worker for some specific interface. We already do this when we close a worker. When a worker implements the AutoClosable interface, it can handle its shutdown event. We could invoke some clean method, or something like this, when we clean a worker dest dir.
We need to specify, if a worker is allowed to re-create the dest dir on its own, or if is has to use the target context API to do that.

@lihaoyi WDYT?

[roman-mibex-2]

Stopping all workers can be costly, especially closing the ubiquitously used ZincWorker would lead to drastically increased build times after a clean

Ok, that would be a cost I personally am willing to pay. For me clean is something went wrong, lets start over from scratch. Ideally I would never need it, but I do once in a while:

• There seems some cases with incremental Zinc builds where it starts screwing up things, especially with removed classes.
• I probably can make my webpack integration not 100% correct for my incremental build =)

For me even an acceptable alternative is something like an extra flag: ./mill clean --stop-server.
It can be as coarse grained as stopping/restarting the background server. After a clean --stop-server starting up the background server and warming it up again is perfectly fine price I'm willing to pay.

[lefou]

I don't questioned the need to reset/restart a worker that needs a dest folder when cleaning stuff. I'm just exploring the solution space before we change something.

As a work around, you can run mill shutdown to stop the server, which will also stop all workers.

[lihaoyi]

I think clean shutting down workers makes sense

[lefou]

Yeah, it's a clean and easy concept. Users can selectively clean, if they want to keep workers. But even if we have some dest-independent worker, users may want to reset them, and clean already has these semantics.

[lefou]

I created an issue: #3276