Implement Server.start_serving() and related APIs

[jlaine]

This is still work in progress, but I wanted to have some code up so we can start discussing it.

• I have moved backlog to a member, so that listen (internal API for start_serving) can be called without arguments.

• I noticed some argument validation (e.g.: backlog, ssl, ssl_backlog, timeouts) is performed quite deep inside the code, inside UVStreamServer. Shouldn't this be done closer to the API surface?

• Currently one test fails because the previous code caught an OSError and re-raised it with a different message. Is this a behaviour we want to preserve?

• I'm having trouble understanding the Server class, which seem to serve as the user-facing API as opposed to the internal UVStreamServer subclasses. The bit that bothers we: why does Server have a list of servers, can there really be anything other than 0 or 1?

• Who should carry the internal is_serving state, the Server or the UVStreamServer?

• How should we implement the future returned by serve_forever?

[1st1]

This is still work in progress, but I wanted to have some code up so we can start discussing it.

Thank you! Sorry for the late review, I was OOO for about 10 days. I'll try to address all your questions below:

• I have moved backlog to a member, so that listen (internal API for start_serving) can be called without arguments.

OK.

• I noticed some argument validation (e.g.: backlog, ssl, ssl_backlog, timeouts) is performed quite deep inside the code, inside UVStreamServer. Shouldn't this be done closer to the API surface?

Sure. If necessary we can always add an assertion at a lower level for clarity.

• Currently one test fails because the previous code caught an OSError and re-raised it with a different message. Is this a behaviour we want to preserve?

Usually we try to match (exactly) OSErrors that Python asyncio is raising, down to the message of the exception. Is that the case in the previous code?

• I'm having trouble understanding the Server class, which seem to serve as the user-facing API as opposed to the internal UVStreamServer subclasses.

It is a user-facing API: the object is returned from the create_server() coroutine.

Things that have the UV prefix in the handles directory are Python high-level wrappers around low-level libuv concepts.

The bit that bothers we: why does Server have a list of servers, can there really be anything other than 0 or 1?

Yes, there can be many. If you create a server on localhost it can create more than one server; in the most simple case, you'll have one for IPv4 and one for IPv6.

In asyncio implementation (see CPython/Lib/asyncio/base_events.py for the reference) the Server object has a list of Python sockets that the server listens on. In uvloop, we have a list of libuv's uv_tcp_t handles wrapped in UVStreamServer objects. We later call uv_listen on them to start accepting connections.

• Who should carry the internal is_serving state, the Server or the UVStreamServer?

The Server objects, as it is a user-facing API that controls a bunch of low-level libuv handles.

• How should we implement the future returned by serve_forever?

I think we should be able to copy/paste asyncio implementation from base_events.py.

[jlaine]

• Currently one test fails because the previous code caught an OSError and re-raised it with a different message. Is this a behaviour we want to preserve?

Usually we try to match (exactly) OSErrors that Python asyncio is raising, down to the message of the exception. Is that the case in the previous code?

AFAICT, cpython only wraps OSError when calling sock.bind():

https://github.com/python/cpython/blob/bb3c05d7efca8d23bf39bc2640297ba2598899f3/Lib/asyncio/base_events.py#L940

.. but not when calling sock.listen():

https://github.com/python/cpython/blob/bb3c05d7efca8d23bf39bc2640297ba2598899f3/Lib/asyncio/base_events.py#L290

The bit that bothers we: why does Server have a list of servers, can there really be anything other than 0 or 1?

Yes, there can be many. If you create a server on localhost it can create more than one server; in the most simple case, you'll have one for IPv4 and one for IPv6.

OK sorry for the noise, I missed the for loop.

• How should we implement the future returned by serve_forever?

I think we should be able to copy/paste asyncio implementation from base_events.py.

Indeed, I did just that.

[jlaine]

I had another look at "why are the OSError messages different", and it looks like the reason is:

• The call to uv_tcp_bind succeeds even if there is already a socket bound to the same address / port.

• The call to uv_listen however fails if there is already a socket bound to the same address / port.

The libuv docs for uv_tcp_bind mention this explicitly:

When the port is already taken, you can expect to see an UV_EADDRINUSE error from either uv_tcp_bind(), uv_listen() or uv_tcp_connect(). That is, a successful call to this function does not guarantee that the call to uv_listen() or uv_tcp_connect() will succeed as well.

This is very different from plain asyncio, which bombs as soon as it attempts socket.bind(). This API difference used not to be visible to users, as the bind / listen steps were run in immediate succession, but with the introduction of start_serving it becomes important. How do you suggest we proceed?

[1st1]

Oh well... I guess the only sure option we have is to rewrite this part:

uvloop/uvloop/loop.pyx

Lines 1658 to 1696 in d8fe153

	reuse_port = bool(reuse_port)
	if reuse_port and not has_SO_REUSEPORT:
	raise ValueError(
	'reuse_port not supported by socket module')
	if host == '':
	hosts = [None]
	elif (isinstance(host, str) or not isinstance(host, col_Iterable)):
	hosts = [host]
	else:
	hosts = host
	fs = [self._getaddrinfo(host, port, family,
	uv.SOCK_STREAM, 0, flags,
	0) for host in hosts]
	infos = await aio_gather(*fs, loop=self)
	completed = False
	try:
	for info in infos:
	addrinfo = (<AddrInfo>info).data
	while addrinfo != NULL:
	if addrinfo.ai_family == uv.AF_UNSPEC:
	raise RuntimeError('AF_UNSPEC in DNS results')
	tcp = self._create_server(
	addrinfo.ai_addr, protocol_factory, server,
	ssl, reuse_port, backlog,
	ssl_handshake_timeout, ssl_shutdown_timeout)
	server._add_server(<TCPServer>tcp)
	addrinfo = addrinfo.ai_next
	completed = True
	finally:
	if not completed:
	server.close()

to create Python sockets and bind them in the same way asyncio does that here:

https://github.com/python/cpython/blob/8a1657b93469580ef345c7c91738587f3d76e87d/Lib/asyncio/base_events.py#L1338-L1393

After we have Python sockets bound, we can wrap them in TCPServer objects the same way we do it here:

uvloop/uvloop/loop.pyx

Lines 1708 to 1713 in d8fe153

	tcp = TCPServer.new(self, protocol_factory, server, ssl,
	uv.AF_UNSPEC,
	ssl_handshake_timeout, ssl_shutdown_timeout)
	try:
	tcp._open(sock.fileno())

That's the only way I can think of that will preserve full compatibility with asyncio. Performance-wise this shouldn't have any impact.

[jlaine]

OK sounds like a plan!

[jlaine]

@1st1 I have implemented what you suggested, but I'm now getting a warning about an unclosed resource:

uvloop/tests/test_tcp.py:967: ResourceWarning: unclosed <socket.socket fd=11, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('0.0.0.0', 43443)>
  None, 0, backlog=bl)

What do we need to do with the Python socket object once we have wrapped it in a TCPServer?

Edit: the problem was in fact due to TCPServer.new raising an exception (in this case validating the backlog argument) which left the socket unclosed.

[jlaine]

Hi Yury, what's the commit policy for uvloop, specifically who is expected to actually do the merge to master? The reviewer, author, .. ?

[1st1]

You can merge it yourself. Please rebase it on top of recent master before merging though (has now flake8 integration)

[jlaine]

OK I've rebased on top of master and fixed 2 flake8 errors. If CI completes successfully I'll merge it.

[denismakogon]

When a new release with these changes will be published?

[1st1]

Need to implement sendfile support first.

[denismakogon]

Any estimates for that?

[1st1]

Unfortunately I don't have time to tackle that myself in the next couple of weeks. Hopefully we'll get it in March.

[1st1]

Thanks so much, @jlaine!