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

Return real result in Web API /code/run #1426

Closed
xsebek opened this issue Aug 13, 2023 · 6 comments · Fixed by #2108
Closed

Return real result in Web API /code/run #1426

xsebek opened this issue Aug 13, 2023 · 6 comments · Fixed by #2108
Assignees
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Moderate The fix or feature would substantially improve user experience. T-Web Involves the web interface - generally communicating with Swarm via ports. Z-Feature A new feature to be added to the game. Z-User Experience This issue seeks to make the game more enjoyable to play.

Comments

@xsebek
Copy link
Member

xsebek commented Aug 13, 2023

Currently we don't wait for the evaluation to finish:

❯ curl --header "Content-Type: text/plain;charset=utf-8" --data "move" localhost:5357/code/run
Sent

Getting back the result would make it possible to control Swarm remotely... or at least from different window.

@xsebek xsebek added Z-User Experience This issue seeks to make the game more enjoyable to play. Z-Feature A new feature to be added to the game. C-Moderate Effort Should take a moderate amount of time to address. S-Moderate The fix or feature would substantially improve user experience. T-Web Involves the web interface - generally communicating with Swarm via ports. labels Aug 13, 2023
@xsebek
Copy link
Member Author

xsebek commented Aug 13, 2023

@byorgey @kostmo I am not sure what the best way to pass back the result from AppState to Web API is.

Maybe an MVar (Either Text Text) would do, as the web could just wait for it to be filled by the controller.

@kostmo
Copy link
Member

kostmo commented Aug 14, 2023

Perhaps this:

For each /code/run request that the web API makes, the web process generates a unique integer ID. This ID gets passed as an additional field of the RunWebCode constructor.

When the game process receives the request, it shall populate a game-internal Map of these integers to their execution statuses (which can be one of two possible constructors: InProgress or Complete <result>).

The web process then uses a different web API endpoint to poll the state of the game-internal execution-status Map. A web UI would use this to display "progress spinners", updating at intervals of, say, 1 second. Alternatively (and probably less usefully), "blocking" could be done within the web process until the command-Complete state is observed for the requested ID, with some timeout (also bad, because the execution time of a command can be arbitrarily long).

@xsebek
Copy link
Member Author

xsebek commented Aug 15, 2023

@kostmo there is a type in Servant for sending values as they are produced - StreamGet.

I played with it and it's not too bad - just do the blocking IO in Effect, send result in Yield and then either Stop or recurse.

I think this would be more efficient, AFAIK we do not allow running other commands while the last one is not finished. If that is so, should we really have a Map if there is only one? We could also add that UID to REPL history. 🤔

@kostmo
Copy link
Member

kostmo commented Aug 15, 2023

we do not allow running other commands while the last one is not finished

Correct, and in fact now we could indicate this circumstance to the caller, by providing a Rejected state:

data RejectionReason = AlreadyRunning | ParseError String
data WebInvocationState = Rejected RejectionReason | InProgress | Complete Value

The more I think about it, the less appealing it seems to do blocking in the web server. I'm not sure I fully understand StreamGet, but I like that the state of the swarm game, thus far, is strictly read-only via IO. Also I'm thinking about edge cases in the web browser, such as possibly having multiple tabs open for the same game, that would accidentally allow multiple overlapping commands to be issued.

@xsebek
Copy link
Member Author

xsebek commented Aug 15, 2023

such as possibly having multiple tabs open for the same game, that would accidentally allow multiple overlapping commands to be issued

@kostmo I don't understand how that would happen as in that case we would just reject it.

Anyway I like the datatype suggestions and agree it would be nice to keep the state readonly if possible.

I have to read more Servant docs to determine which path is better, but I hope one MVar might be a small price for good user experience. I will try to hack on it over the weekend and we shall see what works.

@byorgey
Copy link
Member

byorgey commented Jun 19, 2024

it would be nice to keep the state readonly if possible

I would go further and say we must keep the state read-only at all costs! There should be a single code path for updating the game state --- the event handler. Anything else would just be asking for trouble.

xsebek added a commit that referenced this issue Aug 11, 2024
* send InProgress or Rejected
* once REPLDone send Complete
* closes #1426
@mergify mergify bot closed this as completed in #2108 Aug 15, 2024
@mergify mergify bot closed this as completed in 1994ab2 Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Moderate The fix or feature would substantially improve user experience. T-Web Involves the web interface - generally communicating with Swarm via ports. Z-Feature A new feature to be added to the game. Z-User Experience This issue seeks to make the game more enjoyable to play.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants