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 Ctrl+C and tests on Windows. #1808

Merged
merged 29 commits into from
Mar 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
33388e6
Fix Ctrl+C on Windows.
Tronic Mar 18, 2020
6ea8183
Disable testing of a function N/A on Windows.
Tronic Mar 18, 2020
68fb963
Add test for coverage, avoid crash on missing _stopping.
Tronic Mar 18, 2020
da26adf
Initialise StreamingHTTPResponse.protocol = None
Tronic Mar 20, 2020
1580ca6
Improved comments.
Tronic Mar 20, 2020
2356165
Merge branch 'typing-fix' into ctrlc-windows
Tronic Mar 20, 2020
6f536a1
Reduce amount of data in test_request_stream to avoid failures on Win…
Tronic Mar 21, 2020
77a0e04
Merge branch 'py38-windows-test' into ctrlc-windows
Tronic Mar 21, 2020
ae03fce
The Windows test doesn't work on Windows :(
Tronic Mar 21, 2020
fda9556
Use port numbers more likely to be free than 8000.
Tronic Mar 21, 2020
1418003
Merge branch 'py38-windows-test' into ctrlc-windows
Tronic Mar 21, 2020
5f8fc0c
Disable the other signal tests on Windows as well.
Tronic Mar 21, 2020
036c1d7
Windows doesn't properly support SO_REUSEADDR, so that's disabled in …
Tronic Mar 21, 2020
dc5d682
app.run argument handling: added server kwargs (alike create_server),…
Tronic Mar 21, 2020
46484eb
Revert "app.run argument handling: added server kwargs (alike create_…
Tronic Mar 22, 2020
6f55780
Use random test server port on most tests. Should avoid port/addr reu…
Tronic Mar 22, 2020
73a2416
Another test to random port instead of 8000.
Tronic Mar 22, 2020
66ff770
Fix deprecation warnings about missing name on Sanic() in tests.
Tronic Mar 22, 2020
ed17a3c
Linter and typing
Tronic Mar 22, 2020
41dff40
Increase test coverage
Tronic Mar 22, 2020
f58319f
Rewrite test for ctrlc_windows_workaround
Tronic Mar 22, 2020
fd2ba55
py36 compat
Tronic Mar 22, 2020
d3c8dd4
py36 compat
Tronic Mar 23, 2020
8514d1f
py36 compat
Tronic Mar 23, 2020
2b8a8dc
Don't rely on loop internals but add a stopping flag to app.
Tronic Mar 23, 2020
1257bd3
App may be restarted.
Tronic Mar 23, 2020
1c40860
py36 compat
Tronic Mar 23, 2020
b4f47e2
Linter
Tronic Mar 23, 2020
b3a38b7
Add a constant for OS checking.
Tronic Mar 25, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion sanic/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def __init__(
self.sock = None
self.strict_slashes = strict_slashes
self.listeners = defaultdict(list)
self.is_stopping = False
self.is_running = False
self.is_request_stream = False
self.websocket_enabled = False
Expand Down Expand Up @@ -1177,6 +1178,7 @@ def run(

try:
self.is_running = True
self.is_stopping = False
Copy link
Member

Choose a reason for hiding this comment

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

What's the diff between is_running and is_stopping ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if workers > 1 and os.name != "posix":
logger.warn(
f"Multiprocessing is currently not supported on {os.name},"
Expand Down Expand Up @@ -1209,7 +1211,9 @@ def run(

def stop(self):
"""This kills the Sanic"""
get_event_loop().stop()
if not self.is_stopping:
self.is_stopping = True
get_event_loop().stop()

async def create_server(
self,
Expand Down
27 changes: 27 additions & 0 deletions sanic/compat.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import asyncio
import signal

from sys import argv

from multidict import CIMultiDict # type: ignore
Expand All @@ -23,3 +26,27 @@ def stat_async(path):

async def open_async(file, mode="r", **kwargs):
return aio_open(file, mode, **kwargs)


def ctrlc_workaround_for_windows(app):
async def stay_active(app):
"""Asyncio wakeups to allow receiving SIGINT in Python"""
while not die:
# If someone else stopped the app, just exit
if app.is_stopping:
return
# Windows Python blocks signal handlers while the event loop is
# waiting for I/O. Frequent wakeups keep interrupts flowing.
await asyncio.sleep(0.1)
# Can't be called from signal handler, so call it from here
app.stop()

def ctrlc_handler(sig, frame):
nonlocal die
if die:
raise KeyboardInterrupt("Non-graceful Ctrl+C")
die = True

die = False
signal.signal(signal.SIGINT, ctrlc_handler)
app.add_task(stay_active)
1 change: 1 addition & 0 deletions sanic/response.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ def __init__(
self.headers = Header(headers or {})
self.chunked = chunked
self._cookies = None
self.protocol = None

async def write(self, data):
"""Writes a chunk of data to the streaming response.
Expand Down
18 changes: 8 additions & 10 deletions sanic/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from httptools import HttpRequestParser # type: ignore
from httptools.parser.errors import HttpParserError # type: ignore

from sanic.compat import Header
from sanic.compat import Header, ctrlc_workaround_for_windows
from sanic.exceptions import (
HeaderExpectationFailed,
InvalidUsage,
Expand All @@ -37,6 +37,8 @@
except ImportError:
pass

OS_IS_WINDOWS = os.name == "nt"


class Signal:
stopped = False
Expand Down Expand Up @@ -929,15 +931,11 @@ def serve(

# Register signals for graceful termination
if register_sys_signals:
_singals = (SIGTERM,) if run_multiple else (SIGINT, SIGTERM)
for _signal in _singals:
try:
loop.add_signal_handler(_signal, loop.stop)
except NotImplementedError:
logger.warning(
"Sanic tried to use loop.add_signal_handler "
"but it is not implemented on this platform."
)
if OS_IS_WINDOWS:
ctrlc_workaround_for_windows(app)
else:
for _signal in [SIGTERM] if run_multiple else [SIGINT, SIGTERM]:
loop.add_signal_handler(_signal, app.stop)
pid = os.getpid()
try:
logger.info("Starting worker [%s]", pid)
Expand Down
10 changes: 7 additions & 3 deletions sanic/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

ASGI_HOST = "mockserver"
HOST = "127.0.0.1"
PORT = 42101
PORT = None


class SanicTestClient:
Expand Down Expand Up @@ -95,14 +95,15 @@ async def error_handler(request, exception):

if self.port:
server_kwargs = dict(
host=host or self.host, port=self.port, **server_kwargs
host=host or self.host, port=self.port, **server_kwargs,
)
host, port = host or self.host, self.port
else:
sock = socket()
sock.bind((host or self.host, 0))
server_kwargs = dict(sock=sock, **server_kwargs)
host, port = sock.getsockname()
self.port = port

if uri.startswith(
("http:", "https:", "ftp:", "ftps://", "//", "ws:", "wss:")
Expand All @@ -114,6 +115,9 @@ async def error_handler(request, exception):
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}/")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


@self.app.listener("after_server_start")
async def _collect_response(sanic, loop):
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

Why 0 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

super().__init__(dispatch=dispatch, base_url=base_url)

self.last_request = None
Expand Down
12 changes: 11 additions & 1 deletion tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import pytest

from sanic import Sanic
from sanic.exceptions import SanicException
from sanic.response import text

Expand Down Expand Up @@ -48,6 +49,7 @@ def test_asyncio_server_no_start_serving(app):
if not uvloop_installed():
loop = asyncio.get_event_loop()
asyncio_srv_coro = app.create_server(
port=43123,
return_asyncio_server=True,
asyncio_server_kwargs=dict(start_serving=False),
)
Expand All @@ -61,6 +63,7 @@ def test_asyncio_server_start_serving(app):
if not uvloop_installed():
loop = asyncio.get_event_loop()
asyncio_srv_coro = app.create_server(
port=43124,
return_asyncio_server=True,
asyncio_server_kwargs=dict(start_serving=False),
)
Expand Down Expand Up @@ -199,10 +202,17 @@ def handler(request):

with caplog.at_level(logging.ERROR):
request, response = app.test_client.get("/")
port = request.server_port
assert port > 0
assert response.status == 500
assert "Mock SanicException" in response.text
assert (
"sanic.root",
logging.ERROR,
"Exception occurred while handling uri: 'http://127.0.0.1:42101/'",
f"Exception occurred while handling uri: 'http://127.0.0.1:{port}/'",
) in caplog.record_tuples


def test_app_name_required():
with pytest.deprecated_call():
Sanic()
2 changes: 1 addition & 1 deletion tests/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ async def test_request_class_custom():
class MyCustomRequest(Request):
pass

app = Sanic(request_class=MyCustomRequest)
app = Sanic(name=__name__, request_class=MyCustomRequest)

@app.get("/custom")
def custom_request(request):
Expand Down
12 changes: 6 additions & 6 deletions tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,42 +44,42 @@ def test_load_from_object_string_exception(app):

def test_auto_load_env():
environ["SANIC_TEST_ANSWER"] = "42"
app = Sanic()
app = Sanic(name=__name__)
assert app.config.TEST_ANSWER == 42
del environ["SANIC_TEST_ANSWER"]


def test_auto_load_bool_env():
environ["SANIC_TEST_ANSWER"] = "True"
app = Sanic()
app = Sanic(name=__name__)
assert app.config.TEST_ANSWER == True
del environ["SANIC_TEST_ANSWER"]


def test_dont_load_env():
environ["SANIC_TEST_ANSWER"] = "42"
app = Sanic(load_env=False)
app = Sanic(name=__name__, load_env=False)
assert getattr(app.config, "TEST_ANSWER", None) is None
del environ["SANIC_TEST_ANSWER"]


def test_load_env_prefix():
environ["MYAPP_TEST_ANSWER"] = "42"
app = Sanic(load_env="MYAPP_")
app = Sanic(name=__name__, load_env="MYAPP_")
assert app.config.TEST_ANSWER == 42
del environ["MYAPP_TEST_ANSWER"]


def test_load_env_prefix_float_values():
environ["MYAPP_TEST_ROI"] = "2.3"
app = Sanic(load_env="MYAPP_")
app = Sanic(name=__name__, load_env="MYAPP_")
assert app.config.TEST_ROI == 2.3
del environ["MYAPP_TEST_ROI"]


def test_load_env_prefix_string_value():
environ["MYAPP_TEST_TOKEN"] = "somerandomtesttoken"
app = Sanic(load_env="MYAPP_")
app = Sanic(name=__name__, load_env="MYAPP_")
assert app.config.TEST_TOKEN == "somerandomtesttoken"
del environ["MYAPP_TEST_TOKEN"]

Expand Down
2 changes: 1 addition & 1 deletion tests/test_custom_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def body_finish(self):


def test_custom_request():
app = Sanic(request_class=CustomRequest)
app = Sanic(name=__name__, request_class=CustomRequest)

@app.route("/post", methods=["POST"])
async def post_handler(request):
Expand Down
4 changes: 2 additions & 2 deletions tests/test_keep_alive_timeout.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@

from sanic import Sanic, server
from sanic.response import text
from sanic.testing import HOST, PORT, SanicTestClient

from sanic.testing import HOST, SanicTestClient

CONFIG_FOR_TESTS = {"KEEP_ALIVE_TIMEOUT": 2, "KEEP_ALIVE": True}

old_conn = None
PORT = 42101 # test_keep_alive_timeout_reuse doesn't work with random port


class ReusableSanicConnectionPool(
Expand Down
60 changes: 57 additions & 3 deletions tests/test_logging.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import os
import uuid

from importlib import reload
Expand All @@ -12,6 +13,7 @@
from sanic import Sanic
from sanic.log import LOGGING_CONFIG_DEFAULTS, logger
from sanic.response import text
from sanic.testing import SanicTestClient


logging_format = """module: %(module)s; \
Expand Down Expand Up @@ -127,7 +129,7 @@ async def conn_lost(request):
def test_logger(caplog):
rand_string = str(uuid.uuid4())

app = Sanic()
app = Sanic(name=__name__)

@app.get("/")
def log_info(request):
Expand All @@ -137,15 +139,67 @@ def log_info(request):
with caplog.at_level(logging.INFO):
request, response = app.test_client.get("/")

port = request.server_port

# Note: testing with random port doesn't show the banner because it doesn't
# define host and port. This test supports both modes.
if caplog.record_tuples[0] == (
"sanic.root",
logging.INFO,
f"Goin' Fast @ http://127.0.0.1:{port}",
):
caplog.record_tuples.pop(0)

assert caplog.record_tuples[0] == (
"sanic.root",
logging.INFO,
f"http://127.0.0.1:{port}/",
)
assert caplog.record_tuples[1] == ("sanic.root", logging.INFO, rand_string)
assert caplog.record_tuples[-1] == (
"sanic.root",
logging.INFO,
"Server Stopped",
)


def test_logger_static_and_secure(caplog):
# Same as test_logger, except for more coverage:
# - test_client initialised separately for static port
# - using ssl
rand_string = str(uuid.uuid4())

app = Sanic(name=__name__)

@app.get("/")
def log_info(request):
logger.info(rand_string)
return text("hello")

current_dir = os.path.dirname(os.path.realpath(__file__))
ssl_cert = os.path.join(current_dir, "certs/selfsigned.cert")
ssl_key = os.path.join(current_dir, "certs/selfsigned.key")

ssl_dict = {"cert": ssl_cert, "key": ssl_key}

test_client = SanicTestClient(app, port=42101)
with caplog.at_level(logging.INFO):
request, response = test_client.get(
f"https://127.0.0.1:{test_client.port}/",
server_kwargs=dict(ssl=ssl_dict),
)

port = test_client.port

assert caplog.record_tuples[0] == (
"sanic.root",
logging.INFO,
"Goin' Fast @ http://127.0.0.1:42101",
f"Goin' Fast @ https://127.0.0.1:{port}",
)
assert caplog.record_tuples[1] == (
"sanic.root",
logging.INFO,
"http://127.0.0.1:42101/",
f"https://127.0.0.1:{port}/",
)
assert caplog.record_tuples[2] == ("sanic.root", logging.INFO, rand_string)
assert caplog.record_tuples[-1] == (
Expand Down
6 changes: 3 additions & 3 deletions tests/test_logo.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ def test_logo_false(app, caplog):
loop.run_until_complete(_server.wait_closed())
app.stop()

banner, port = caplog.record_tuples[ROW][2].rsplit(":", 1)
assert caplog.record_tuples[ROW][1] == logging.INFO
assert caplog.record_tuples[ROW][
2
] == f"Goin' Fast @ http://127.0.0.1:{PORT}"
assert banner == "Goin' Fast @ http://127.0.0.1"
assert int(port) > 0


def test_logo_true(app, caplog):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_request_stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sanic.views import stream as stream_decorator


data = "abc" * 10000000
data = "abc" * 1_000_000


def test_request_stream_method_view(app):
Expand Down
Loading