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

Fix reloader on OSX py38 and Windows #1827

Merged
merged 25 commits into from
Jun 3, 2020
Merged

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Mar 31, 2020

Simplified a bit and this now appears to work on all OS. On Windows we don't get graceful shutdowns though, due to lack of SIGTERM.

Also removes app.run(..., **kwargs) that was silently consuming invalid arguments, and forces using keywords for all args but host and port (potentially breaking some apps).

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #1827 into master will increase coverage by 2.62%.
The diff coverage is 85.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1827      +/-   ##
==========================================
+ Coverage   92.17%   94.79%   +2.62%     
==========================================
  Files          23       24       +1     
  Lines        2314     2307       -7     
  Branches      424      421       -3     
==========================================
+ Hits         2133     2187      +54     
+ Misses        141       75      -66     
- Partials       40       45       +5     
Impacted Files Coverage Δ
sanic/reloader_helpers.py 75.40% <82.35%> (+60.67%) ⬆️
sanic/app.py 93.02% <100.00%> (+0.76%) ⬆️
sanic/__main__.py 71.87% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4658e0f...b636083. Read the comment docs.

@Tronic
Copy link
Member Author

Tronic commented Mar 31, 2020

This includes #1555 but due to bigger and potentially breaking changes will need more testing. One known issue is that if the server does not exit, the autoreloader will get stuck waiting until another Ctrl+C, after which it will probably abandon the server process and any worker processes that it may have, because this no longer uses external tools to kill all children. This should be fixed in server code, making it exit and terminate its children in a more reliable manner on SIGTERM (for instance, making non-graceful exit if not completed within a timeout). SIGKILL on the server is not a good idea, especially so in multiple worker mode, where the workers would be left running.

I believe that eventually autoreloader should be merged to the main process of multiworker mode but that will require a lot more work. Another consideration is that with workers=1 possibly the main process should run the server and the autoreloader be its child, to allow normal debugging etc.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

I have tested the PR to make sure it still works on Linux. I have not pulled it to confirm in OSX or Windows yet.

sanic/app.py Show resolved Hide resolved
@ahopkins ahopkins merged commit 230941f into sanic-org:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants