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

WebHost: properly stop worker threads #3340

Merged
merged 3 commits into from
May 19, 2024
Merged

WebHost: properly stop worker threads #3340

merged 3 commits into from
May 19, 2024

Conversation

black-sliver
Copy link
Member

@black-sliver black-sliver commented May 19, 2024 •
edited
Loading

What is this fixing or adding?

In the current code

  • with SELFHOST:
    • killing the main thread will hang waiting for the worker threads to stop.
    • this has the side effect of multiprocessing workers to receive Ctrl+C, die and respawn because the pool never stops
    • pressing Ctrl+C again will stop waiting for the worker threads and just terminate
  • without SELFHOST:
    • similar to above, but we immediately enter the state where we wait for the threads to stop, so only one Ctrl+C is required to actually terminate the process

After this change:

  • with SELHOST:
    • we gracefully stop the worker threads once waitress finishes
    • the application terminates cleanly (with just 1x Ctrl+C)
  • without SELHOST:
    • similar to above, but we spin the main thread ourself until SIGINT is received

How was this tested?

Only with waitress locally and with SELFHOST: false locally.

@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
LegendaryLinux
LegendaryLinux approved these changes May 19, 2024
View reviewed changes
Copy link
Member

@LegendaryLinux LegendaryLinux left a comment

Choose a reason for hiding this comment

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

Confirmed this fixes an issue on PyCharm in Windows wherein you must click the "stop process" button twice to end the WebHost process.

@black-sliver black-sliver merged commit e978109 into ArchipelagoMW:main May 19, 2024
16 checks passed
@black-sliver black-sliver deleted the stop-worker-threads branch May 19, 2024 18:40
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
@black-sliver
WebHost: properly stop worker threads (ArchipelagoMW#3340)
dc31082 
* WebHost: properly stop worker threads

* Less jank

* Forgot the try-catch around the while true
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
@black-sliver @sflavelle
WebHost: properly stop worker threads (ArchipelagoMW#3340)
50f9f08 
* WebHost: properly stop worker threads

* Less jank

* Forgot the try-catch around the while true
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
@black-sliver @qwint
WebHost: properly stop worker threads (ArchipelagoMW#3340)
5d4a36a 
* WebHost: properly stop worker threads

* Less jank

* Forgot the try-catch around the while true
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

@LegendaryLinux LegendaryLinux LegendaryLinux approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

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