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

krun-server should outlive first command #33

Closed
teohhanhui opened this issue Jun 7, 2024 · 4 comments · Fixed by #34
Closed

krun-server should outlive first command #33

teohhanhui opened this issue Jun 7, 2024 · 4 comments · Fixed by #34

Comments

@teohhanhui
Copy link
Collaborator

teohhanhui commented Jun 7, 2024

(Continuing from https://github.com/slp/krun/pull/23#discussion_r1624008269)

With some real-world testing, it's entirely jarring behaviour from a user perspective.

Steps for reproduction:

  1. Launch krun FEXBash
  2. Launch krun FEXInterpreter steam
  3. exit from FEXBash

Expected result:
Steam should continue running.

Actual / observed behaviour:
Steam is killed abruptly.

@slp
Copy link
Collaborator

slp commented Jun 7, 2024

Something we can do, in krun-server, is keep track of the children and, if there's one that's still alive when the main command exits, inform the user and keep waiting until Ctrl+C is hit, or there are no more children running.

This behavior would also work well with binfmt integration. First command run, even from a Desktop link, will start up the VM, which will be kept alive until the last command exits.

WDYT?

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented Jun 7, 2024

Sounds reasonable. I'll send a PR.

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented Jun 8, 2024

Did some digging:
https://users.rust-lang.org/t/how-to-properly-close-a-tcplistener-in-multi-thread-server/87376

Track requests, not threads.

https://users.rust-lang.org/t/how-to-properly-close-a-tcplistener-in-multi-thread-server/87376/17

shutdown does not affect accept, only recv... But OTOH, we don't need to care about the server thread being blocked on accept, as long as all in-flight requests are handled (and also wait for child processes to exit)... So, we in fact do NOT want to join the server thread.

We also don't want to use Drop as it's not unwind safe:

@teohhanhui
Copy link
Collaborator Author

teohhanhui commented Jun 8, 2024

Okay, I think we really need to rethink our architecture here. This is not going to work in any user friendly way.

Waiting for children to exit still means new krun invocations in the meantime would fail for no apparent reason. That's very bad UX.

It seems to me the only possible solution without involving a long-lived krun server running in the background would be: delay the shutdown if a new incoming connection is accepted. (If we do anything to stop listening, we're back to the above problem with new krun invocations failing...)

I'll continue thinking and working on 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 a pull request may close this issue.

2 participants