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

Replace Win32 named pipe with simple ServerSocket #1089

Closed

Conversation

sake92
Copy link
Contributor

@sake92 sake92 commented Dec 25, 2020

This currently works fine, but spawns a new server on every invocation!?
I'm not sure if this is a problem introduced by this change but I doubt it.
Maybe I just don't understand the locks code in Mill well enough.. 😄

Anyways, it runs a simple ServerSocket on a random port on localhost, and writes that port in a file.
Client can then read the port and connect to the server.

I also "tested" the security part of server, by running an Android app that scans the ports of my computer.
It doesn't show the allocated port, so should be good.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 26, 2020

One thing missing from this PR is a description of what it is actually trying to accomplish, an understanding of what the behavior and problems of the current system are, and an explanation of how this is the best solution to fix those problems 😛

The client/server code could definitely use a lot of work, particularly on windows, and definitely has bugs. But it's a pretty tricky area of code involving multi-process concurrency and locking, so it demands more "measure twice cut once"-style changes rather than "move fast and break things"

@sake92
Copy link
Contributor Author

sake92 commented Dec 26, 2020

@lihaoyi yeah, that makes sense. Can you describe a bit the logic behind MillServerMain code?
I mean just conceptually, what is its main logic trying to achieve?
Then we could get to some conclusion and improvements maybe? 😄

For example, it's not clear to me why in while (running) { it reopens the server socket?

@lihaoyi
Copy link
Member

lihaoyi commented Dec 27, 2020

The logic is trying to implement the following:

  1. If there is no MillServerMain, or there are existing MillServerMains but they're all in use, we create a new one and connect to it
  2. If there is an existing MillServerMain running that is not currently being used, we connect to it and use it
  3. If there are n=5 existing MillServerMains, and all are in use, we fail with an error (just to stop someone from accidentally spawning 100s of background processes)
  4. An individual MillServerMain terminates after n=5 minutes or inactivity, or if Ctrl-C is given while it is executing tasks
  5. If a MillServerMain is given Ctrl-C while in the file-watching mode, it should not terminate but remain in the background to pick up further tasks

A lot of the complexity around locks etc. is to do this reliably in the presence of multiple Mill clients being spawned concurrently and in arbitrary orders, with the given commands taking arbitrary amounts of time to run

@sake92
Copy link
Contributor Author

sake92 commented Mar 22, 2021

#1230 tries to fix existing code instead of this.

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