Skip to content

Simplify asyncio and server list #2033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Simplify asyncio and server list #2033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0071ac1
eliminate redundant server_close()
alexrudd2 Feb 20, 2024
268f87b
class methods can be used without instancing the class
alexrudd2 Feb 20, 2024
c884f4b
mypy
alexrudd2 Feb 20, 2024
dbe3cd5
eliminate asyncio.sleep() and replace time.sleep() with a timeout
alexrudd2 Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 5 additions & 22 deletions pymodbus/server/async_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import asyncio
import os
import time
import traceback
from contextlib import suppress

Expand Down Expand Up @@ -284,10 +283,6 @@ def callback_new_connection(self):
return ModbusServerRequestHandler(self)

async def shutdown(self):
"""Shutdown server."""
await self.server_close()

async def server_close(self):
"""Close server."""
if not self.serving.done():
self.serving.set_result(True)
Expand Down Expand Up @@ -556,19 +551,14 @@ class _serverList:
:meta private:
"""

active_server: ModbusTcpServer | ModbusUdpServer | ModbusSerialServer

def __init__(self, server):
"""Register new server."""
self.server = server
self.loop = asyncio.get_event_loop()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

active_server: ModbusTcpServer | ModbusUdpServer | ModbusSerialServer | None

@classmethod
async def run(cls, server, custom_functions):
"""Help starting/stopping server."""
for func in custom_functions:
server.decoder.register(func)
cls.active_server = _serverList(server)
cls.active_server = server
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

with suppress(asyncio.exceptions.CancelledError):
await server.serve_forever()

Expand All @@ -577,11 +567,7 @@ async def async_stop(cls):
"""Wait for server stop."""
if not cls.active_server:
raise RuntimeError("ServerAsyncStop called without server task active.")
await cls.active_server.server.shutdown()
if os.name == "nt":
await asyncio.sleep(1)
else:
await asyncio.sleep(0)
await cls.active_server.shutdown()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@alexrudd2 alexrudd2 Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cls.active_server = None

@classmethod
Expand All @@ -593,11 +579,8 @@ def stop(cls):
if not cls.active_server.loop.is_running():
Log.info("ServerStop called with loop stopped.")
return
asyncio.run_coroutine_threadsafe(cls.async_stop(), cls.active_server.loop)
if os.name == "nt":
time.sleep(10)
else:
time.sleep(0.1)
future = asyncio.run_coroutine_threadsafe(cls.async_stop(), cls.active_server.loop)
future.result(timeout=10 if os.name == 'nt' else 0.1)


async def StartAsyncTcpServer( # pylint: disable=invalid-name,dangerous-default-value
Expand Down
12 changes: 6 additions & 6 deletions test/sub_server/test_server_asyncio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ async def _setup_teardown(self):

# teardown
if self.server is not None:
await self.server.server_close()
await self.server.shutdown()
self.server = None
if self.task is not None:
await asyncio.sleep(0.1)
Expand Down Expand Up @@ -239,15 +239,15 @@ async def test_async_tcp_server_connection_lost(self):
BasicClient.transport.close()
await asyncio.sleep(0.2) # so we have to wait a bit

async def test_async_tcp_server_close_connection(self):
"""Test server_close() while there are active TCP connections."""
async def test_async_tcp_server_shutdown_connection(self):
"""Test server shutdown() while there are active TCP connections."""
await self.start_server()
await self.connect_server()

# On Windows we seem to need to give this an extra chance to finish,
# otherwise there ends up being an active connection at the assert.
await asyncio.sleep(0.5)
await self.server.server_close()
await self.server.shutdown()

async def test_async_tcp_server_no_slave(self):
"""Test unknown slave exception."""
Expand All @@ -258,7 +258,7 @@ async def test_async_tcp_server_no_slave(self):
await self.start_server()
await self.connect_server()
assert not BasicClient.eof.done()
await self.server.server_close()
await self.server.shutdown()
self.server = None

async def test_async_tcp_server_modbus_error(self):
Expand Down Expand Up @@ -313,7 +313,7 @@ async def test_async_start_udp_server(self):
async def test_async_udp_server_serve_forever_close(self):
"""Test StarAsyncUdpServer serve_forever() method."""
await self.start_server(do_udp=True)
await self.server.server_close()
await self.server.shutdown()
self.server = None

async def test_async_udp_server_serve_forever_twice(self):
Expand Down