Refactor handling of ProxyStreams and Client-Server Locks

[lihaoyi]

This PR reduces the number of filesystem locks from 3 to 2, removes a potential race condition, fixes an edge case, tries harder to ensure all cleanup steps are run, consolidates and adds docs and fuzz tests to the ProxyStream logic, and reduces the Mill client fixed overhead by ~60ms on my laptop

• Combined ProxyStreamPumper and ProxyOutputStream into a single ProxyStream.java file with two static inner classes ProxyStream.Pumper and ProxyStream.Output. This should help keep the related logic in one place, so it can easily be reviewed and read together in one place.

• Moved the magic 1 and -1 literals into constants ProxyStream.OUT and ProxyStream.ERR

• Introduced a new ProxyStream.END = 0 constant to the protocol, used via ProxyStream.sendEnd, that can be used to terminate the ProxyStream.Pumper gracefully.

• Add some docs and simple fuzz tests for ProxyStream logic, asserting that both separate contents of OUT and ERR streams are round tripped unchanged, and also that the ordering of the combined out and err stream is preserved appropriately.

  • Fix an edge case bug surfaced by the test suite: in the case when the ProxyStream stream/socket is closed before the serverLock is released, this results in the -1 "end of stream" being sent to ProxyStream.Pumper is mistaken for a length-1 packet being written to the ERR stream, causing the attempt to read the next byte to fail. We now properly disambiguate the -1 signalling end-of-stream (received as int -1) from the -1 byte representing some data (received as int 255).

• Adjusted the MillServerLauncher.run termination logic; rather than waiting for serverLock to be released to signal the server has completed and then waiting 50ms for data to finish streaming, we instead call outPumperThread.join() which waits for the ProxyStream.Pumper to terminate either via a ProxyStream.END packet or via the socket being closed.

  • This should remove a potential race condition where the server process releases the serverLock but the logs take more than 50ms to arrive (e.g. due to CPU contention) resulting in logs being dropped.
  • As mentioned earlier, the test suite now ensures outPumperThread.join() is now robust against both graceful shutdown (receiving 0) and non-graceful socket closing (receiving -1), and either case should terminate the pumper after all readable input has been read
  • The server also no longer needs to Thread.sleep(10) after each command is processed, since we do not need to wait for the client to probe the serverLock anymore

• Removed serverLock and WaitForSilence.java since they are no longer used

• Move a bunch of server-side termination logic into finally blocks: calling ProxyStream.sendEnd, sock.close(), serverSocket.close(), all live in finally blocks to try to best-effort ensure they always run.

• Converted some java.nio.file.* callsites in the Scala code to os.* for consistency with the rest of the Scala code

• This overall effect of this PR results in a hot ./mill resolve _ in the Mill repo dropping from ~435ms to ~375ms on my laptop, presumably due to the removals of Thread.sleeps and various polling loops

Pull request: #3368

[lefou]

Looks good to me. We need to search our issues, I think this PR might close one.