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

#404 REPL :stop command fails for overly long segments #418

Merged
merged 3 commits into from
Apr 17, 2022

Conversation

ksiyuan
Copy link

@ksiyuan ksiyuan commented Apr 16, 2022

I changed the function hasPlayer so that it checks whether server.player is not an empty struct instead of only checking if server.player.Port is equal to 0. After that it seems the issue never comes out on my computer but I don't know the exact reason.

Besides, to give user a choice to shut down all player process, I added another repl command called stopall which shut down all player process while didn't change the original stop command so that the user can choose whether to shut down the current player or simply shut down everything if something wrong happens.

Changed function `hasPlayer`
Add a new REPL command `stopall` that stops all player process
@daveyarwood
Copy link
Member

Thanks for the PR, but I think I would prefer another approach where we make sure that server.player doesn't end up being set to something unexpected. The improvements in this PR are nice, but they feel more like workarounds than a solution to whatever the problem is.

Assuming my theory here is correct: If the current player is valid but there happens to be an error reading its state file and the port is unexpectedly set to 0, then we should leave server.player alone and continue to interact with that same player process. I have a hunch that that might fix it 🤞

I think there are two ways we could do this (and maybe it makes sense to do both?)

  1. In the places where we set server.player, we should check that the port isn't 0 and avoid setting it if that's the case.
  2. In ReadPlayerStates, we probably shouldn't include any player states where there was a read error in the results.

Adding check when the server.player is set, if the new player/updated state has port = 0, leave the current server.player unchanged.

If there is a read error in ReadPlayerStates, return nil and the read error instead of  including any player states
@ksiyuan
Copy link
Author

ksiyuan commented Apr 17, 2022

I tried both way.

  1. Check if the new player/updatedState has port = 0 and leave the current server.player unchanged if that's the case

  2. In ReadPlayerStates, simply return nil as well as readError when there was a read error in the results.

Not sure if that solves all the problems, but it seems to work on my computer.

@daveyarwood
Copy link
Member

Thanks for looking into this! 🙌

I'm going to make some small adjustments after merging, and get this into a release soon so that we can collectively confirm whether the problem goes away.

@daveyarwood daveyarwood merged commit 8eb4fd6 into alda-lang:master Apr 17, 2022
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.

3 participants