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

response return value runtime check #3321

Merged
merged 16 commits into from
Oct 5, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGES/3304.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Bracket IPv6 addresses in the HOST header
2 changes: 2 additions & 0 deletions CHANGES/3307.doc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Make server access log format placeholder %b documentation reflect
behavior and docstring.
2 changes: 2 additions & 0 deletions CONTRIBUTORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Brian Muller
Bryce Drennan
Carl George
Cecile Tonglet
Colin Dunklau
Chien-Wei Huang
Chih-Yuan Chen
Chris AtLee
Expand Down Expand Up @@ -162,6 +163,7 @@ Paul Colomiets
Paulius Šileikis
Paulus Schoutsen
Pavel Kamaev
Pavel Polyakov
Pawel Miech
Pepe Osca
Philipp A.
Expand Down
2 changes: 2 additions & 0 deletions aiohttp/client_reqrep.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ def update_auto_headers(self, skip_auto_headers):
# add host
if hdrs.HOST not in used_headers:
netloc = self.url.raw_host
if helpers.is_ipv6_address(netloc):
netloc = '[{}]'.format(netloc)
if not self.url.is_default_port():
netloc += ':' + str(self.url.port)
self.headers[hdrs.HOST] = netloc
Expand Down
27 changes: 16 additions & 11 deletions aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
from pathlib import Path
from types import TracebackType
from typing import (TYPE_CHECKING, Any, Callable, Dict, Iterable, Iterator,
List, Mapping, Optional, Tuple, Type, TypeVar, Union, cast)
List, Mapping, Optional, Pattern, Tuple, Type, TypeVar,
Union, cast)
from urllib.parse import quote
from urllib.request import getproxies

Expand Down Expand Up @@ -604,25 +605,29 @@ def __set__(self, inst: Any, value: Any) -> None:
_ipv6_regexb = re.compile(_ipv6_pattern.encode('ascii'), flags=re.IGNORECASE)


def is_ip_address(
host: Optional[Union[str, bytes, bytearray, memoryview]]) -> bool:
def _is_ip_address(
regex: Pattern, regexb: Pattern,
host: Optional[Union[str, bytes, bytearray, memoryview]])-> bool:
if host is None:
return False
if isinstance(host, str):
if _ipv4_regex.match(host) or _ipv6_regex.match(host):
return True
else:
return False
return bool(regex.match(host))
elif isinstance(host, (bytes, bytearray, memoryview)):
if _ipv4_regexb.match(host) or _ipv6_regexb.match(host): # type: ignore # noqa
return True
else:
return False
return bool(regexb.match(host))
else:
raise TypeError("{} [{}] is not a str or bytes"
.format(host, type(host)))


is_ipv4_address = functools.partial(_is_ip_address, _ipv4_regex, _ipv4_regexb)
is_ipv6_address = functools.partial(_is_ip_address, _ipv6_regex, _ipv6_regexb)


def is_ip_address(
host: Optional[Union[str, bytes, bytearray, memoryview]]) -> bool:
return is_ipv4_address(host) or is_ipv6_address(host)


_cached_current_datetime = None
_cached_formatted_datetime = None

Expand Down
11 changes: 9 additions & 2 deletions aiohttp/web_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from .tcp_helpers import tcp_cork, tcp_keepalive, tcp_nodelay
from .web_exceptions import HTTPException
from .web_request import BaseRequest
from .web_response import Response
from .web_response import Response, StreamResponse


__all__ = ('RequestHandler', 'RequestPayloadError', 'PayloadAccessError')
Expand Down Expand Up @@ -346,6 +346,7 @@ async def start(self):
handler = self._task_handler
manager = self._manager
keepalive_timeout = self._keepalive_timeout
resp = None

while not self._force_close:
if not self._messages:
Expand Down Expand Up @@ -389,6 +390,12 @@ async def start(self):
"please raise the exception instead",
DeprecationWarning)

if self.debug:
if not isinstance(resp, StreamResponse):
self.log_debug("Possibly missing return "
"statement on request handler")
raise RuntimeError('Web-handler should \
Copy link
Member

Choose a reason for hiding this comment

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

Please replace \ with implicit strings concatenation as a line above.

return a response instance, got {!r}'.format(resp))
await resp.prepare(request)
await resp.write_eof()

Expand Down Expand Up @@ -438,7 +445,7 @@ async def start(self):
self.log_exception('Unhandled exception', exc_info=exc)
self.force_close()
finally:
if self.transport is None:
if self.transport is None and resp is not None:
Copy link
Member

Choose a reason for hiding this comment

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

No need for the check, self.force_close() implies setting self.transport to None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this check prevents the log.debug last value being overwritten by: https:/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-8ac506a3012476288a758f156ca7d7b2R449 and fail the assert: https:/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-98519d3deafe019f57304a557d20531cR542

maybe there's a way to test it w/o relying on the last log.debug value?

Copy link
Member

Choose a reason for hiding this comment

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

Got you.
Let's drop self.log_debug("Possibly missing return statement on request handler") log but log RuntimeError with it's message.

'Web-handler should return a response instance, got {!r}' is a good enough hint to the missing return statement, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, that runtimeError will be bypassed by this one: https:/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-8ac506a3012476288a758f156ca7d7b2R439 another reason why I used log.debug

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Agree

self.log_debug('Ignored premature client disconnection.')
elif not self._force_close:
if self._keepalive and not self._close:
Expand Down
2 changes: 1 addition & 1 deletion docs/logging.rst
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ request and response:
+--------------+---------------------------------------------------------+
| ``%s`` | Response status code |
+--------------+---------------------------------------------------------+
| ``%b`` | Size of response in bytes, excluding HTTP headers |
| ``%b`` | Size of response in bytes, including HTTP headers |
+--------------+---------------------------------------------------------+
| ``%T`` | The time taken to serve the request, in seconds |
+--------------+---------------------------------------------------------+
Expand Down
20 changes: 20 additions & 0 deletions tests/test_client_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,26 @@ def test_host_header_explicit_host_with_port(make_request) -> None:
assert req.headers['HOST'] == 'example.com:99'


def test_host_header_ipv4(make_request) -> None:
req = make_request('get', 'http://127.0.0.2')
assert req.headers['HOST'] == '127.0.0.2'


def test_host_header_ipv6(make_request) -> None:
req = make_request('get', 'http://[::2]')
assert req.headers['HOST'] == '[::2]'


def test_host_header_ipv4_with_port(make_request) -> None:
req = make_request('get', 'http://127.0.0.2:99')
assert req.headers['HOST'] == '127.0.0.2:99'


def test_host_header_ipv6_with_port(make_request) -> None:
req = make_request('get', 'http://[::2]:99')
assert req.headers['HOST'] == '[::2]:99'


def test_default_loop(loop) -> None:
asyncio.set_event_loop(loop)
req = ClientRequest('get', URL('http://python.org/'))
Expand Down
Loading