create_server returns a Server, not AbstractServer

[scottbelden]

I believe this should resolve my current error:

error: Module 'asyncio.base_events' has no attribute 'Server'

Also, looking at the implementation of create_server and a few other functions, it seems like they should return a Server, not an AbstractServer: https://github.com/python/cpython/blob/9cd39b16e2655f748f7aa8d20bca4812da00ba70/Lib/asyncio/base_events.py#L1466-L1476

[JelleZijlstra]

mypy is unhappy with this for reasons I don't fully understand. Maybe if you make these functions use async def these errors will go away.

[scottbelden]

Oh, I wonder if it's because there is another create_server here

typeshed/stdlib/3/asyncio/events.pyi

Lines 121 to 125 in dad16f2

	def create_server(self, protocol_factory: _ProtocolFactory, host: Optional[Union[str, Sequence[str]]] = ..., port: int = ..., *,
	family: int = ..., flags: int = ...,
	sock: None = ..., backlog: int = ..., ssl: _SSLContext = ...,
	reuse_address: Optional[bool] = ...,
	reuse_port: Optional[bool] = ...) -> Generator[Any, None, AbstractServer]: ...

that I didn't change. Is it a problem that the abstract event loop returns an AbstractServer and this base loop returns a Server?

[srittau]

create_unix_server() is actually not implemented by BaseEventLoop. Just tested this and it throws a NotImplementedError when called. So they should just be removed from here.

[scottbelden]

Removed. I believe the same is true of create_unix_connection and it should probably be removed as well. I haven't done that yet (as it's a bit of a separate issue than what this PR is intending) but can easily do so if you'd like.

[srittau]

@scottbelden I think removing create_unix_connection as well would be a good idea. But that leaves us with the problem that mypy complains that create_unix_* is missing, because it's abstract in the base class and BaseEventLoop, which is concrete, does not implement it. I see several possible solutions:

1. Restore create_unix_server as a copy of AbstractEventLoops signature and leave create_unix_connection alone. I don't like this, because it doesn't reflect reality.
2. Add ABCMeta as metaclass to BaseEventLoop. That even worse, since BaseEventLoop is clearly not meant to be abstract and this can cause problems down the line.
3. Just add # type: ignore to line 22 to force this error to go away. Works, but it's a bit rough, and means that classes deriving from BaseEventLoop need to do the same.
4. Remove @abstractmethod from AbstractEventLoop.create_unix_server and create_unix_connection (and instead add a comment with an explanation). I prefer this solution as these methods are clearly not abstract in the sense that they need to be implemented by any concrete sub-class (which is what @abstractmethod implies).

I'd like to hear @JelleZijlstra's thoughts about this, though.

[JelleZijlstra]

So if I understand correctly, the problem is:

• The base abstract class, AbstractEventLoop, has an abstract method create_unix_server
• BaseEventLoop is meant to be concrete but does not actually implement create_unix_server.

But the second part isn't true: BaseEventLoop has a number of unimplemented methods, and the actual runtime type of an event loop is asyncio.unix_events._UnixSelectorEventLoop for me (and presumably some other class on Windows). So it should be fine to add ABCMeta to BaseEventLoop.

[srittau]

Ah, if BaseEventLoop is not actually meant to be instantiated, I agree.

[scottbelden]

If I understand correctly, class BaseEventLoop(AbstractEventLoop) should be changed to class BaseEventLoop(AbstractEventLoop, metaclass=ABCMeta). Is this correct? Should the create_unix_* methods still be removed from BaseEventLoop?

[srittau]

@scottbelden Yes to both questions. Hopefully that will resolve the build failure.

[srittau]

(Also, it's easier for us if you don't force-push. We will squash merge anyway.)

[JelleZijlstra]

Could you fix the merge conflict and the Travis failure?

[scottbelden]

Sorry, was away for a bit. I believe I addressed the comments and merged with master. It's running through CI right now.