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 Mill Server code #3370

Merged
merged 34 commits into from
Aug 16, 2024
Merged

Clean up Mill Server code #3370

merged 34 commits into from
Aug 16, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 15, 2024

This PR doesn't really make any major behavioral changes, but does some aggressive refactoring so we can properly test most of the client-server logic without pulling in the entire Mill codebase. We take advantage of this to flesh out ClientServerTests to cover a bunch more edge cases around the client-server interactions without needing to write slow and expensive integration tests.

There is definitely work we can do to improve the whole client/server setup behaviorally, but this PR intentionally avoids major changes. But the refactors and test fixtures we set up in this PR should make any future changes easier than they would have been otherwise, and should make it easier to reproduce/debug/fix any other issues we notice in the client-server interactions

Major Changes

  • Split the Mill-specific parts of the client-server code (in runner.client and runner) from the generic client-server parts (in main.client and main.server). All the generic code has the Mill* prefix removed, e.g. MillServerLauncher is now just ServerLauncher

  • Combined the classes of MillServerMain/Server into just Server

  • Refactor the code in ServerLauncher, pulling commonly threaded parameters into constructor, renaming runMain to the more descriptive acquireLocksAndRun, and fold runMillServer into run

  • Make the Mill server shut down when the folder gets deleted, to avoid leaking processes when you rm -rf out/

  • Flesh out ClientServerTests: we now exercise all the code in ServerLauncher, rather than just the run method. We also now exercise the run-multiple-servers-in-different-folders code path, the client-gets-killed-halfway code path, as well as the shut-down-when-server-folder-gets-deleted code path we just added

  • Added a server.log file inside each of the mill-worker-* folders. This is used in tests to assert that the servers go through the code paths we expect, and will also be useful for debugging misbehaviors in future if we see a server process not doing what we expect

  • Many other minor bugfixes and tweaks to allow testability (e.g. simulating client termination with an exception, stubbing out System.exit so the tests don't kill the test process, etc.)

Pull request: #3370

@lihaoyi lihaoyi marked this pull request as ready for review August 15, 2024 13:36
@lihaoyi lihaoyi requested a review from lefou August 15, 2024 13:36
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not yet reviewed the full change, but your details description look all good and tests all pass, so this looks good to me.

What messages go into the server.log? Are they also streamed to the client or are these internal to the server? WDYT about initializing and using a Slf4j logger in the server?

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 16, 2024

The messages are internal to the server, not shown to the client. The general idea is just so we can keep track of what a server is doing, so if a server gets stuck or exits unexpectedly at least we hopefully have some idea of why it did that (in addition to any stack trace from jstack).

I don't have much experience with slf4j and related projects (log4j, logback), so I don't really have much of an opinion here. But if we do add something we'd need to make sure it is separate from the main stdout/stderr logs, since those get captured and streamed to the client (either via System.setOut/Console.withOut or via the out/mill-worker-*/stdout file for stuff that we can't easily redirect) and this stuff is not.

@lihaoyi lihaoyi merged commit 1722258 into com-lihaoyi:main Aug 16, 2024
30 checks passed
@lefou lefou added this to the 0.12.0 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants