Fix Ctrl+C and tests on Windows. #1808

Tronic · 2020-03-18T14:25:50Z

Alternative signal handling logic for Windows (fix #1753) and allow green CI pipelines again.

Adds app.is_stopping for determining whether app.stop has been called, which is different from app not running.
Most tests use random ports because on Windows CI they would otherwise fail to bind.
Fixed deprecation warnings in tests (Sanic missing name argument).

Tronic · 2020-03-18T14:34:04Z

The code adds a background task that wakes up 10 times a second so that Python can receive signals (otherwise the signal handler is only called once Sanic receives network traffic). This is of course not needed on other operating systems.

The signal handler cannot call app.stop directly because it is in signal handling context (on other OSes loop.add_signal_handler is used but this of course is not available on Windows). So, the handler sets a flag, which will be seen by the background task, causing graceful app exit. Additionally, in case of more than one Ctrl+C being received, it falls back to good old KeyboardInterrupt termination.

It took too many hours to figure all that crap out, because it is not in the docs.

Windows still cannot support multiple workers, so that side has not been considered in this fix.

codecov · 2020-03-18T15:01:32Z

Codecov Report

Merging #1808 into master will increase coverage by 0.11%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
+ Coverage   91.94%   92.05%   +0.11%     
==========================================
  Files          23       23              
  Lines        2309     2342      +33     
  Branches      428      435       +7     
==========================================
+ Hits         2123     2156      +33     
  Misses        143      143              
  Partials       43       43

Impacted Files	Coverage Δ
sanic/app.py	`91.90% <80.00%> (+0.06%)`	⬆️
sanic/compat.py	`86.20% <100.00%> (+16.97%)`	⬆️
sanic/response.py	`98.70% <100.00%> (+<0.01%)`	⬆️
sanic/testing.py	`97.97% <100.00%> (+0.02%)`	⬆️
sanic/request.py	`99.63% <0.00%> (+0.01%)`	⬆️

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 60b4efa...b3a38b7. Read the comment docs.

…dows.

…Python, and thus rebinding fails. For successful testing, reuse port instead.

… added warning on extra kwargs, made auto_reload explicit argument. Another go at Windows tests

…server), added warning on extra kwargs, made auto_reload explicit argument. Another go at Windows tests" This reverts commit dc5d682.

…se issues.

Tronic · 2020-03-23T12:24:08Z

Okay, I think I'm finished with this. Care to review?

yunstanford

Thanks for the fix!

yunstanford · 2020-03-24T17:03:48Z

sanic/testing.py

@@ -203,7 +207,7 @@ def __init__(

        self.app = app

-        dispatch = SanicASGIDispatch(app=app, client=(ASGI_HOST, PORT))
+        dispatch = SanicASGIDispatch(app=app, client=(ASGI_HOST, PORT or 0))


Typing says it needs to be int, so None causes a failure. Either the type hint should be changed to Optional[int] or 0 be used instead but I did not look at ASGI code to see how this is actually handled. In any case, 0 is the standard way to request for a free random port.

Actually all of Sanic should probably be changed to accept 0 port. Currently entering zero gives port 8000. But that is a potentially breaking change, and for another PR even if implemented.

yunstanford · 2020-03-24T17:05:31Z

sanic/server.py

-                    "Sanic tried to use loop.add_signal_handler "
-                    "but it is not implemented on this platform."
-                )
+        if os.name == "nt":


Let's define a constant and use that instead of string literal directly

I've added a constant in server.py but did not change this in skipif of OS-dependent tests.

Do note that os.name is the more coarse platform definition where all Windows are "nt", where sys.platform uses many names for Windows (and quite confusingly, 64 bit Python on 64 bit Windows is still called "win32").

yunstanford · 2020-03-24T17:05:59Z

sanic/app.py

@@ -1177,6 +1178,7 @@ def run(

        try:
            self.is_running = True
+            self.is_stopping = False


What's the diff between is_running and is_stopping ?

When the app has not yet started, both are False. After calling app.stop the app may keep running for a while, so in that period both are True. Asyncio implementation uses loop._stopping in a similar fashion but uvloop doesn't have that and depending on private parts of the implementation is ugly in any case.

yunstanford · 2020-03-24T17:07:55Z

sanic/testing.py

@@ -114,6 +115,9 @@ def _collect_request(request):
            url = "{scheme}://{host}:{port}{uri}".format(
                scheme=scheme, host=host, port=port, uri=uri
            )
+        # Tests construct URLs using PORT = None, which means random port not
+        # known until this function is called, so fix that here
+        url = url.replace(":None/", f":{port}/")


a little bit concern on this, probably should avoid this kinda of hacky pattern.

Avoiding this would require major restructuring of testing.py which I am not quite up to, or rewriting/disabling all tests that depend on this.

Fix Ctrl+C on Windows.

33388e6

Disable testing of a function N/A on Windows.

6ea8183

Tronic added 9 commits March 18, 2020 17:19

Add test for coverage, avoid crash on missing _stopping.

68fb963

Initialise StreamingHTTPResponse.protocol = None

da26adf

Improved comments.

1580ca6

Merge branch 'typing-fix' into ctrlc-windows

2356165

Reduce amount of data in test_request_stream to avoid failures on Win…

6f536a1

…dows.

Merge branch 'py38-windows-test' into ctrlc-windows

77a0e04

The Windows test doesn't work on Windows :(

ae03fce

Use port numbers more likely to be free than 8000.

fda9556

Merge branch 'py38-windows-test' into ctrlc-windows

1418003

Tronic changed the title ~~Fix Ctrl+C on Windows.~~ Fix Ctrl+C and tests on Windows. Mar 21, 2020

Tronic mentioned this pull request Mar 21, 2020

Avoid py38 test failure on Windows #1816

Closed

Tronic added 15 commits March 21, 2020 20:01

Disable the other signal tests on Windows as well.

5f8fc0c

Windows doesn't properly support SO_REUSEADDR, so that's disabled in …

036c1d7

…Python, and thus rebinding fails. For successful testing, reuse port instead.

app.run argument handling: added server kwargs (alike create_server),…

dc5d682

… added warning on extra kwargs, made auto_reload explicit argument. Another go at Windows tests

Revert "app.run argument handling: added server kwargs (alike create_…

46484eb

…server), added warning on extra kwargs, made auto_reload explicit argument. Another go at Windows tests" This reverts commit dc5d682.

Use random test server port on most tests. Should avoid port/addr reu…

6f55780

…se issues.

Another test to random port instead of 8000.

73a2416

Fix deprecation warnings about missing name on Sanic() in tests.

66ff770

Linter and typing

ed17a3c

Increase test coverage

41dff40

Rewrite test for ctrlc_windows_workaround

f58319f

py36 compat

fd2ba55

py36 compat

d3c8dd4

py36 compat

8514d1f

Don't rely on loop internals but add a stopping flag to app.

2b8a8dc

App may be restarted.

1257bd3

Tronic added 2 commits March 23, 2020 14:06

py36 compat

1c40860

Linter

b4f47e2

Tronic mentioned this pull request Mar 23, 2020

Initialise StreamingHTTPResponse.protocol = None #1815

Closed

yunstanford reviewed Mar 24, 2020

View reviewed changes

Add a constant for OS checking.

b3a38b7

yunstanford approved these changes Mar 26, 2020

View reviewed changes

yunstanford merged commit 120f026 into sanic-org:master Mar 26, 2020

Tronic deleted the ctrlc-windows branch March 27, 2020 15:10

Tronic mentioned this pull request May 22, 2020

asyncio signal handlers don't work on Windows #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ctrl+C and tests on Windows. #1808

Fix Ctrl+C and tests on Windows. #1808

Tronic commented Mar 18, 2020 •

edited

Loading

Tronic commented Mar 18, 2020

codecov bot commented Mar 18, 2020 •

edited

Loading

Tronic commented Mar 23, 2020

yunstanford left a comment

yunstanford Mar 24, 2020

Tronic Mar 25, 2020

Tronic Mar 25, 2020

yunstanford Mar 24, 2020

Tronic Mar 25, 2020 •

edited

Loading

yunstanford Mar 24, 2020

Tronic Mar 25, 2020

yunstanford Mar 24, 2020

Tronic Mar 25, 2020

Fix Ctrl+C and tests on Windows. #1808

Fix Ctrl+C and tests on Windows. #1808

Conversation

Tronic commented Mar 18, 2020 • edited Loading

Tronic commented Mar 18, 2020

codecov bot commented Mar 18, 2020 • edited Loading

Codecov Report

Tronic commented Mar 23, 2020

yunstanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tronic Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tronic commented Mar 18, 2020 •

edited

Loading

codecov bot commented Mar 18, 2020 •

edited

Loading

Tronic Mar 25, 2020 •

edited

Loading