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 https://github.com/pymodbus-dev/pymodbus/issues/1992#issue-2128311471) ==> replace with waiting+timeout for the Future

[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 😆 https://github.com/pymodbus-dev/pymodbus/blob/fe809d9685a15f2b476b396d1dbee0cfca93b56f/pymodbus/server/async_io.py#L571

[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 https://github.com/pymodbus-dev/pymodbus/pull/2033/commits/dbe3cd5626ef841f07d106b3e3073665f54be23c 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 https://github.com/pymodbus-dev/pymodbus/pull/2033/commits/0071ac11d0e050b2e168b86e6cde42898855e1a8?

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[bot]]

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[bot]]

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