Simplify asyncio and server list

[alexrudd2]

Note: this code is very confusing to me, so @janiversen please review carefully.

Solves another 5 mypy errors, and speeds up the Window tests.

Instancing the class _serverList from its own method
(1) is... strange?
(2) is unnecessary to use the methods, which are all @classmethod
(3) is unnecessary to keep track of the event loop, since every server class already has self.loop
==> Use the class methods directly

The methods shutdown() and server_close() are essentially redundant.
==> combine

There's no use for asyncio.wait() directly after an await.
==> remove

time.sleep() is inefficient, as described in #1992 (comment))
==> replace with waiting+timeout for the Future

[janiversen]

It looks to me as you have misunderstood what _serverlist does.

_serverlist maintains a list of running servers, allowing us to shutdown all tasks.

[janiversen]

We do need to register the server, that is sort of the whole purpose of the class (keep track of multiple servers).

it used in line 596 and can not be undefined (its interesting that the linters do not complain in line 596, since the class no longer have a loop variable.

[alexrudd2]

They do not complain because each server has already has self.loop

pymodbus/pymodbus/server/async_io.py

Line 267 in fe809d9

self.loop = asyncio.get_running_loop()

[janiversen]

aah...but we still need to register the server (or somehow have a list somewhere).

[janiversen]

This is how we build a list of servers, shortcutting that means no list of servers.

[janiversen]

active_server is a _serverlist object, not the real server (name is quite confusing though).

[alexrudd2]

This comment and below "These are needed to allow the async task to run" got confused because the diff view does not show the changes well.

Really the changes are this:

-- await cls.active_server.server.shutdown()
++ await cls.active_server.shutdown()

The new code shuts down a server correctly since active_server is now a server object.

await cls.active_server.shutdown()
-- if os.name == "nt":
--  await asyncio.sleep(1)
-- else:
--  await asyncio.sleep(0)

I do not see why the sleeps are "needed to allow the asyncio tasks to run". The await cls.active_server.shutdown() already yields control flow back to the event loop and the await asyncio.sleep() is redundant, no?

[janiversen]

We have had severe problems in the past, but not forcing a task switch which is essentially what asyncio.sleep(0) does. Not having a task switch mean e.g. that the task is cancelled but not yet terminated, not always but sometimes (especially on windows).

[janiversen]

So you are saying there are no reason to keep a list of active servers, but then we could remove _server_list totally.

_server_list was build to maintain a list of active server, which comes handy when debugging...I do not like to keep _server_list without the list.

[alexrudd2]

We do need to register the server, that is sort of the whole purpose of the class (keep track of multiple servers).

active_server is a _serverlist object, not the real server (name is quite confusing though).

I was indeed confused here. The name suggests it's a list, but the code looks more like a mixin than a list. (I'm still not sure how the class method run() can instance its own class! **) Each server already knows the event loop (via self.loop in ModbusBaseServer) and handles its own start/stop (via Start/StopServer).

How do multiple servers work?

If it was something like below I would understand.

class _serverlist:
def __init__(self):
    self.servers = []
    self.active_server = None

async def run(cls, server, custom_functions):
   self.servers.append(server)
   self.active_server = server

**this is the most confusing code I have seen in a long time 😆

pymodbus/pymodbus/server/async_io.py

Line 571 in fe809d9

cls.active_server = _serverList(server)

[alexrudd2]

I considered making this a discussion/issue, but think you prefer to talk about the code. You can consider this PR a draft, I guess. However, it does function ... somehow :)

Each commit is separate, so I'll cherry-pick dbe3cd5 as a separate PR.

[janiversen]

I understand your confusion, this is code that took quite a while to get right.

StartServer can be called multiple times (we start a async loop each time and call StartAsyncServer)
StartAsyncServer can be called multiple times, but do not return unless terminated !

ServerClose and ServerAsyncClose closes the newest started server, but remark it is not a class method, so it have no way of knowing which servers are running... That is where _server_list is used.

_server_list "remembers" the servers started in a class variable.

Run instantiates an object of itself, that is a pattern you see in a lot of code.

[janiversen]

To be quite blunt the convenience functions Start* Close* are not what I like, I find it much more correct to instantiate the server object, and work directly.

However removing these function caused quite a fuzz years ago, so they remain, until I find a bigger excuse to make 4.0

[alexrudd2]

Do you want me to split out 0071ac1?

So you are saying there are no reason to keep a list of active servers, but then we could remove _server_list totally.
_server_list was build to maintain a list of active server, which comes handy when debugging...I do not like to keep _server_list without the list.

I do not know enough to have a preference here. Hopefully I have sparked enough discussion for you to improve this some other way.

[janiversen]

Yes please. As we go down the road _server_list needs to die.

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions]

This PR was closed because it has been stalled for 10 days with no activity.