bpo-32662: Implement Server.start_serving() and Server.serve_forever()

[1st1]

New methods:

• Server.start_serving(),
• Server.serve_forever(), and
• Server.is_serving().

Add 'start_serving' keyword parameter to loop.create_server() and
loop.create_unix_server().

https://bugs.python.org/issue32662

[asvetlov]

Documentation is needed

[asvetlov]

Backward incompatible change.
I pretty sure 99.9% of users newer modified the attribute but we should have a big warning about it.

[1st1]

There was no point in modifying this attribute ever, but I agree, it needs to be documented. Will fix in the documentation.

[asvetlov]

It is loop's private method call.
For me it's a bad smell.
Can we implement the feature with public API calls only?

[1st1]

No, we can't, without re-architecturing half of create server machinery and exposing a new event loop public API. I have no interest in doing that.

And I don't see why this is a problem -- Loop and Server are designed to work together. Loop creates Server objects and knows everything about them. This is exactly the same relationship as between Transports and Loops.

Also you can't use Server object from uvloop and vice versa, so I don't see any problem here either.

[asvetlov]

Ok, I don't insist. We have many tightly coupled classes in asyncio already.

[asvetlov]

Parenthesis instead of backslash looks better.

[1st1]

Fixed

[asvetlov]

You forgot about self._serving_forever_fut.cancelled() check.

[1st1]

Good catch!

[1st1]

Wait... done() includes the cancelled() check, so adding it would have no effect... Cancelled futures are "done", so if a future is not "done" it means that it's not "cancelled".

[asvetlov]

Sorry, you are right.

[asvetlov]

Should check self._serving attribute too maybe.

[1st1]

No, we document this method as idempotent as related to server already accepting connections. I.e. it's fine to do this:

await server.start_serving()
await server.serve_forever()

The reason for this design is backwards-compatibility: loop.create_server by default returns a Server object that's already serving! So server.serve_forever() must be idempotent.

I found the utility in server.start_serving() when I was writing unittests. Sometimes we need to deterministically know when a server just started to listen, otherwise, for instance, it would be impossible for me to write a working loop.create_unix_server unittest.

[asvetlov]

Thanks for explanation

[asvetlov]

serve_forever() coroutine calls self.close() but doesn't wait for actual closing (await self.wait_closed()).
It is strange and complicated the usage: the proper code should always be:

try:
    await server.serve_forever()
except Exception:
     await server.wait_closed()
     raise

Moreover server.sockets contains listening sockets only, long running accepted connections should be closed separately. I think it should be mentioned in documentation. Unfortunately I see no way how to fix the problem quickly (and the fix is out of scope of the PR, sure).

[1st1]

serve_forever() coroutine calls self.close() but doesn't wait for actual closing (await self.wait_closed()).

OK, will do that.

Moreover server.sockets contains listening sockets only, long running accepted connections should be closed separately. I think it should be mentioned in documentation. Unfortunately I see no way how to fix the problem quickly (and the fix is out of scope of the PR, sure).

Yes, I agree :( Need to fix that in 3.8.

[asvetlov]

Let's discuss it later. Actually at least in aiohttp we need stop listening first (server.close() does the job perfectly) and close all already accepted connections in controlled way (with signals for graceful shutdown and close timeout).

[asvetlov]

Looks good.
Do you prefer to write documentation about new methods in this PR or want a separate one (documentation can be added in Python beta stage).

[1st1]

@asvetlov Andrew, I've pushed an update with docs and addressed review comments. Please take a look.

[asvetlov]

LGTM