search

alda-lang / alda

A music programming language for musicians. :notes:
https://alda.io
Eclipse Public License 2.0
5.58k stars 282 forks source link

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

Closed ksiyuan closed 2 years ago

ksiyuan commented 2 years ago

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.

daveyarwood commented 2 years ago

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 :crossed_fingers:

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.
ksiyuan commented 2 years ago

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 commented 2 years ago

Thanks for looking into this! :raised_hands:

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.