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

WebHost: fix AutoLauncher restarting rooms due to race condition #3333

Merged
merged 5 commits into from
May 19, 2024

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

Rooms are spinning too much

How was this tested?

locally

If this makes graphical changes, please attach screenshots.

@Berserker66 Berserker66 requested a review from black-sliver May 19, 2024 11:38
@github-actions github-actions bot added 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 May 19, 2024
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Hard to make sure this works on live webhost, but the +5 should be fine and locker import is dead in customserver.

WebHostLib/autolauncher.py Outdated Show resolved Hide resolved
@black-sliver
Copy link
Member

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Another thing I just realized is that rooms_shutting_down.put(room_id) won't run if there is an exception during shutdown (re-raise or in finally). So the room(s) then gets stuck, Maybe move rooms_shutting_down to finally and wrap the db_session in the finally block in a try-except-log?

WebHostLib/customserver.py Outdated Show resolved Hide resolved
WebHostLib/customserver.py Outdated Show resolved Hide resolved
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Should be fine? Everything I could think of was addressed. Maybe test the final incarnation before pushing live?

@Berserker66 Berserker66 merged commit 663b50b into main May 19, 2024
26 checks passed
@Berserker66 Berserker66 deleted the webhost_autolauncher_fix branch May 19, 2024 13:18
Berserker66 added a commit that referenced this pull request May 19, 2024
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants