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

Use selectors lib to address select() fd num limit #301

Merged
merged 33 commits into from
Jul 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
702670e
server: reproduce select() out of range error
tommilligan Jul 17, 2020
3e9ac1f
feedback: move imports, make finally robust
tommilligan Jul 17, 2020
0edbd0f
feedback: don't change hard_limit when adjusting nofile resource
tommilligan Jul 17, 2020
bcae452
feedback: add assertions to enforce we reach a file-descriptor over 1024
tommilligan Jul 17, 2020
53e8a5f
make test_high_number_of_file_descriptors py27 compatible
tommilligan Jul 17, 2020
0fad950
connections: replace select with selectors/selectors2
tommilligan Jul 17, 2020
919d563
only run test_high_number_of_file_descriptors on unix
tommilligan Jul 17, 2020
7047f0b
really only run test_high_number_of_file_descriptors on unix
tommilligan Jul 17, 2020
6ca52e0
feedback: use closing contextmanager for socket
tommilligan Jul 17, 2020
85b800d
Update cheroot/test/test_server.py
tommilligan Jul 19, 2020
e69b260
Update cheroot/connections.py
tommilligan Jul 19, 2020
d273ae9
Update cheroot/connections.py
tommilligan Jul 19, 2020
b664203
Update cheroot/test/test_server.py
tommilligan Jul 19, 2020
c4aec4f
Update cheroot/test/test_server.py
tommilligan Jul 19, 2020
153e666
Update cheroot/test/test_server.py
tommilligan Jul 19, 2020
513ed26
Update cheroot/test/test_server.py
tommilligan Jul 19, 2020
ff26fdf
Apply suggestions from code review
tommilligan Jul 19, 2020
ee7261b
move selectors import to compat
tommilligan Jul 19, 2020
5fbbb9e
integrate feedback with tests, make work again
tommilligan Jul 19, 2020
bdc33cb
Use `http_server` fixture in `test_high_number_of_file_descriptors`
webknjaz Jul 19, 2020
aea6bf9
Improve `test_high_number_of_file_descriptors` docstring
webknjaz Jul 19, 2020
84f8616
Add LGTM ignore comments to `_compat`
webknjaz Jul 19, 2020
38a4ec3
Revert "Use `http_server` fixture in `test_high_number_of_file_descri…
webknjaz Jul 19, 2020
ef5d44d
Move high fd num test prerequisites to fixtures
webknjaz Jul 20, 2020
5617f53
Fix marking `select()` as code in docstring
webknjaz Jul 20, 2020
24f5d13
Add "syscall" to the list of known words
webknjaz Jul 20, 2020
d0c56f2
Only install pytest-forked where it's supported
webknjaz Jul 20, 2020
ba258dd
Apply updated add-trailing-comma edits
webknjaz Jul 20, 2020
c8dea9b
Conditionally decorate test with forked mark
webknjaz Jul 20, 2020
72252db
Bump setup-python GHA to v2.1.1
webknjaz Jul 21, 2020
e996f20
Add selectors34 fallback for py27 under 64-bit Win
webknjaz Jul 21, 2020
dc2c17d
Revert "Add selectors34 fallback for py27 under 64-bit Win"
webknjaz Jul 21, 2020
f15243f
Revert "Bump setup-python GHA to v2.1.1"
webknjaz Jul 21, 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: 6 additions & 0 deletions cheroot/_compat.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# pylint: disable=unused-import
"""Compatibility code for using Cheroot with various versions of Python."""

from __future__ import absolute_import, division, print_function
Expand All @@ -8,6 +9,11 @@

import six

try:
import selectors # lgtm [py/unused-import]
except ImportError:
import selectors2 as selectors # noqa: F401 # lgtm [py/unused-import]

try:
import ssl
IS_ABOVE_OPENSSL10 = ssl.OPENSSL_VERSION_INFO >= (1, 1)
Expand Down
23 changes: 17 additions & 6 deletions cheroot/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

import io
import os
import select
import socket
import time

from . import errors
from ._compat import selectors
from .makefile import MakeFile

import six
Expand Down Expand Up @@ -62,6 +62,7 @@ def __init__(self, server):
"""
self.server = server
self.connections = []
self._selector = selectors.DefaultSelector()

def put(self, conn):
"""Put idle connection into the ConnectionManager to be managed.
Expand Down Expand Up @@ -133,10 +134,12 @@ def get_conn(self, server_socket):
ss_fileno = server_socket.fileno()
socket_dict[ss_fileno] = server_socket
try:
rlist, _, _ = select.select(list(socket_dict), [], [], 0.1)
# No available socket.
if not rlist:
return None
for fno in socket_dict:
self._selector.register(fno, selectors.EVENT_READ)
Copy link
Contributor

Choose a reason for hiding this comment

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

not strictly necessary, but it would be preferable to avoid re-registering and unregistering all sockets each time this select()s - each register() and unregister() will typically incur a syscall per socket, which reduces performance unnecessarily, particularly with large numbers of sockets.

the selector already maintains a map of which sockets are registered - see get_map() for instance, and the SelectorKey allows storing application-specific state in the data field if desired.

this may require a bit more reorganization than the current patch, so could be deferred to a separate PR (if the performance impact is acceptable), but should be considered in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @liamstask! Makes sense. @tommilligan WDYT? Mind addressing this?

rlist = [
key.fd for key, _event
in self._selector.select(timeout=0.1)
]
except OSError:
# Mark any connection which no longer appears valid.
for fno, conn in list(socket_dict.items()):
Expand All @@ -155,14 +158,22 @@ def get_conn(self, server_socket):

# Wait for the next tick to occur.
return None
finally:
for fno in socket_dict:
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
self._selector.unregister(fno)

try:
# See if we have a new connection coming in.
rlist.remove(ss_fileno)
except ValueError:
# No new connection, but reuse existing socket.
# If we didn't get any readable sockets, wait for the next tick
if not rlist:
return None

# No new connection, but reuse an existing socket.
conn = socket_dict[rlist.pop()]
else:
# If we have a new connection, reuse the server socket
conn = server_socket

# All remaining connections in rlist should be marked as ready.
Expand Down
106 changes: 105 additions & 1 deletion cheroot/test/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import absolute_import, division, print_function
__metaclass__ = type

from contextlib import closing
import os
import socket
import tempfile
Expand All @@ -19,7 +20,7 @@
from six.moves import urllib

from .._compat import bton, ntob
from .._compat import IS_LINUX, IS_MACOS, SYS_PLATFORM
from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM
from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPServer
from ..testing import (
ANY_INTERFACE_IPV4,
Expand Down Expand Up @@ -236,3 +237,106 @@ def test_peercreds_unix_sock_with_lookup(peercreds_enabled_server):
peercreds_text_resp = requests.get(unix_base_uri + PEERCRED_TEXTS_URI)
peercreds_text_resp.raise_for_status()
assert peercreds_text_resp.text == expected_textcreds


@pytest.mark.skipif(
IS_WINDOWS,
reason='This regression test is for a Linux bug, '
'and the resource module is not available on Windows',
)
@pytest.mark.parametrize(
'resource_limit',
(
1024,
2048,
),
indirect=('resource_limit',),
)
@pytest.mark.usefixtures('many_open_sockets')
def test_high_number_of_file_descriptors(resource_limit):
"""Test the server does not crash with a high file-descriptor value.

This test shouldn't cause a server crash when trying to access
file-descriptor higher than 1024.

The earlier implementation used to rely on ``select()`` syscall that
doesn't support file descriptors with numbers higher than 1024.
"""
# We want to force the server to use a file-descriptor with
# a number above resource_limit

# Create our server
httpserver = HTTPServer(
bind_addr=(ANY_INTERFACE_IPV4, EPHEMERAL_PORT), gateway=Gateway,
)
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
httpserver.prepare()

try:
# This will trigger a crash if select() is used in the implementation
httpserver.tick()
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
except: # noqa: E722
raise # only needed for `else` to work
else:
# We use closing here for py2-compat
with closing(socket.socket()) as sock:
tommilligan marked this conversation as resolved.
Show resolved Hide resolved
# Check new sockets created are still above our target number
assert sock.fileno() >= resource_limit
finally:
# Stop our server
httpserver.stop()
webknjaz marked this conversation as resolved.
Show resolved Hide resolved


if not IS_WINDOWS:
test_high_number_of_file_descriptors = pytest.mark.forked(
test_high_number_of_file_descriptors,
)


@pytest.fixture
def resource_limit(request):
"""Set the resource limit two times bigger then requested."""
resource = pytest.importorskip(
'resource',
reason='The "resource" module is Unix-specific',
)

# Get current resource limits to restore them later
soft_limit, hard_limit = resource.getrlimit(resource.RLIMIT_NOFILE)

# We have to increase the nofile limit above 1024
# Otherwise we see a 'Too many files open' error, instead of
# an error due to the file descriptor number being too high
resource.setrlimit(
resource.RLIMIT_NOFILE,
(request.param * 2, hard_limit),
)

try: # noqa: WPS501
yield request.param
finally:
# Reset the resource limit back to the original soft limit
resource.setrlimit(resource.RLIMIT_NOFILE, (soft_limit, hard_limit))


@pytest.fixture
def many_open_sockets(resource_limit):
"""Allocate a lot of file descriptors by opening dummy sockets."""
# Hoard a lot of file descriptors by opening and storing a lot of sockets
test_sockets = []
# Open a lot of file descriptors, so the next one the server
# opens is a high number
try:
for i in range(resource_limit):
sock = socket.socket()
test_sockets.append(sock)
# If we reach a high enough number, we don't need to open more
if sock.fileno() >= resource_limit:
break
# Check we opened enough descriptors to reach a high number
the_highest_fileno = test_sockets[-1].fileno()
assert the_highest_fileno >= resource_limit
yield the_highest_fileno
finally:
# Close our open resources
for test_socket in test_sockets:
test_socket.close()
1 change: 1 addition & 0 deletions docs/spelling_wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ ssl
stdout
subclasses
submodules
syscall
systemd
threadpool
Tidelift
Expand Down
3 changes: 3 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ setup_requires =
# These are required in actual runtime:
install_requires =
backports.functools_lru_cache; python_version < '3.3'
selectors2; python_version< '3.4'
six>=1.11.0
more_itertools>=2.6
jaraco.functools
Expand All @@ -85,6 +86,8 @@ docs =

testing =
pytest>=4.6.6
pytest-forked>=1.1.3; sys_platform != "win32" and python_version >= '3.0' and python_version <= '3.4'
pytest-forked>=1.2.0; sys_platform != "win32" and (python_version < '3.0' or python_version > '3.4')
pytest-mock>=1.11.0
pytest-sugar>=0.9.3
pytest-testmon<1.0.0
Expand Down