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.

Sign up for GitHub

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

Jump to bottom

MultiServer, customserver, CI, Test: Fix problems in room hosting and test/simulate it #3464

Merged
merged 10 commits into from
Jun 5, 2024
Merged

MultiServer, customserver, CI, Test: Fix problems in room hosting and test/simulate it #3464

merged 10 commits into from
Jun 5, 2024

Conversation

black-sliver
Copy link
Member

@black-sliver black-sliver commented Jun 4, 2024 •
edited
Loading

What is this fixing or adding?

  • Fixes various things in MultiServer and customserver:
    • auto-save thread could in theory run forever
    • auto-save thread could overwrite save (in practice, the room timeout is >1min, so this won't do anything)
    • /exit would not cleanly shutdown, because cancelling a task raises one level higher
    • Shutdown rooms would not free memory because they are never awaited / .result()ed
    • Exceptions could be hidden (same reason as above)
    • Fix data package not being consistent (with *_name_groups)
    • Ctrl+C on autohoster would not save rooms
  • Add some type hints that helped while writing the code
  • Add test for the above, run with python test/hosting
  • Add CI for the test

How was this tested?

  • I added a test util and added that to CI

What's still missing

  • rolling on webhost
  • verifying (and fixing) that autohoster saves on CTRL+C (I think it doesn't)
    • stop_autohost(True) could be used for that already
  • making the final test more resilient to randomness
    • currently, if a thread would overwrite the save, it would be a race that is not always detected
  • a test to check if static_server_data gets mutated

CTRL+C we should do done, the rest doesn't seem that important.
Sadly this took way too long already :/

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 4, 2024
@black-sliver black-sliver added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jun 5, 2024
Berserker66
Berserker66 approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@Berserker66 Berserker66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read over the code, that looks good. I can run it on windows if you'd like. Before I try it on the test webhost I'd like to see the ctrl-c save issue fixed, otherwise we might distort the received indexes of some clients and delete/duplicate some items.

@black-sliver
Copy link
Member Author

If we don't want to test until then, should I push to this PR and re-request review?

@Berserker66
Copy link
Member

sounds good to me.

@black-sliver
Copy link
Member Author

Assuming we don't wait for the _save(True) vs. _save(False), this should be ready for a final review

@black-sliver black-sliver requested a review from Berserker66 June 5, 2024 22:14
Berserker66
Berserker66 reviewed Jun 5, 2024
View reviewed changes
WebHostLib/customserver.py
@@ -277,12 +277,15 @@ async def start_room(room_id):
ctx.logger.exception("Could not determine port. Likely hosting failure.")
with db_session:
ctx.auto_shutdown = Room.get(id=room_id).timeout
if ctx.saving:
setattr(asyncio.current_task(), "save", lambda: ctx._save(True))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea one could do this, and admittedly it feels somewhat cursed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really does. I think the alternative would be keeping yet another list of active rooms. Not sure if that would be better, but I feel like it would be another source of memory leaks.

@black-sliver black-sliver merged commit afb6d9c into ArchipelagoMW:main Jun 5, 2024
17 checks passed
@black-sliver black-sliver deleted the simulate-hosting branch June 5, 2024 23:54
wu4 pushed a commit to wu4/Archipelago that referenced this pull request Jun 6, 2024
@black-sliver @wu4
MultiServer, customserver, CI, Test: Fix problems in room hosting and…
bf5e96e 
… test/simulate it (ArchipelagoMW#3464)

* Test: add hosting simulation test

* WebHost: add weak typing to get_app()

* MultiServer: add typing to auto_saver_thread

* MultiServer: don't cancel task, properly end it

* customserver: stop auto-save thread from saving after shutdown

and make sure it stops, another potential memory leak

* MultiServer, customserver: make datapackage small again

* customserver: collect/finish room tasks

Hopefully fixes the memory leak we are seeing

* CI: test hosting

* Test: hosting: verify autohoster saves on Ctrl+C

* customserver: save when stopping via Ctrl+C
agilbert1412 pushed a commit to agilbert1412/Archipelago that referenced this pull request Jun 13, 2024
@black-sliver @agilbert1412
MultiServer, customserver, CI, Test: Fix problems in room hosting and…
b16d9b8 
… test/simulate it (ArchipelagoMW#3464)

* Test: add hosting simulation test

* WebHost: add weak typing to get_app()

* MultiServer: add typing to auto_saver_thread

* MultiServer: don't cancel task, properly end it

* customserver: stop auto-save thread from saving after shutdown

and make sure it stops, another potential memory leak

* MultiServer, customserver: make datapackage small again

* customserver: collect/finish room tasks

Hopefully fixes the memory leak we are seeing

* CI: test hosting

* Test: hosting: verify autohoster saves on Ctrl+C

* customserver: save when stopping via Ctrl+C
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
@black-sliver
MultiServer, customserver, CI, Test: Fix problems in room hosting and…
834e73d 
… test/simulate it (ArchipelagoMW#3464)

* Test: add hosting simulation test

* WebHost: add weak typing to get_app()

* MultiServer: add typing to auto_saver_thread

* MultiServer: don't cancel task, properly end it

* customserver: stop auto-save thread from saving after shutdown

and make sure it stops, another potential memory leak

* MultiServer, customserver: make datapackage small again

* customserver: collect/finish room tasks

Hopefully fixes the memory leak we are seeing

* CI: test hosting

* Test: hosting: verify autohoster saves on Ctrl+C

* customserver: save when stopping via Ctrl+C
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
@black-sliver @sflavelle
MultiServer, customserver, CI, Test: Fix problems in room hosting and…
0c2d434 
… test/simulate it (ArchipelagoMW#3464)

* Test: add hosting simulation test

* WebHost: add weak typing to get_app()

* MultiServer: add typing to auto_saver_thread

* MultiServer: don't cancel task, properly end it

* customserver: stop auto-save thread from saving after shutdown

and make sure it stops, another potential memory leak

* MultiServer, customserver: make datapackage small again

* customserver: collect/finish room tasks

Hopefully fixes the memory leak we are seeing

* CI: test hosting

* Test: hosting: verify autohoster saves on Ctrl+C

* customserver: save when stopping via Ctrl+C
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
@black-sliver @qwint
MultiServer, customserver, CI, Test: Fix problems in room hosting and…
244b63c 
… test/simulate it (ArchipelagoMW#3464)

* Test: add hosting simulation test

* WebHost: add weak typing to get_app()

* MultiServer: add typing to auto_saver_thread

* MultiServer: don't cancel task, properly end it

* customserver: stop auto-save thread from saving after shutdown

and make sure it stops, another potential memory leak

* MultiServer, customserver: make datapackage small again

* customserver: collect/finish room tasks

Hopefully fixes the memory leak we are seeing

* CI: test hosting

* Test: hosting: verify autohoster saves on Ctrl+C

* customserver: save when stopping via Ctrl+C
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Berserker66 Berserker66 Berserker66 approved these changes

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@black-sliver @Berserker66