aiohttp==3.9.0b1 breaks pymodbus tests

[alexrudd2]

There is some sort of change in aiohttp==3.9.0b1 that breaks 2 of the tests.

python3 -m pytest -k simulator_server_tcp .
[...] snip
E       AssertionError: ERROR tasks/threads hanging after 9 retries:
E
E       assert False

aiohttp==3.9.0b0 is fine

[alexrudd2]

Breaking change bisected to aio-libs/aiohttp#7780

[Dreamsorcerer]

Can you link to the actual tests failing? The above comment doesn't show the test, filename, stacktrace or anything. ;)

[alexrudd2]

https://github.com/pymodbus-dev/pymodbus/actions/runs/6805365296/job/18505192239

 =========================== short test summary info ============================
ERROR test/sub_examples/test_examples.py::TestExamples::test_simulator - AssertionError: ERROR tasks/threads hanging after 9 retries:
  
assert False
ERROR test/sub_server/test_simulator.py::TestSimulator::test_simulator_server_tcp - AssertionError: ERROR tasks/threads hanging after 9 retries:
  
assert False

Here's one of the two tests:

pymodbus/test/sub_server/test_simulator.py

Lines 576 to 581 in 1db29af

	async def test_simulator_server_tcp(self):
	"""Test init simulator server."""
	task = ModbusSimulatorServer()
	await task.run_forever(only_start=True)
	await asyncio.sleep(5)
	await task.stop()

I think the actual assert is here:
https://github.com/pymodbus-dev/pymodbus/blob/dev/test/conftest.py#L76
It's a fixture to make sure teardown/cleanup works, but you'll have to ask @janiversen for how it actually works.

[Dreamsorcerer]

Ah, you're starting/stopping the site directly and not the runner. Which atleast means that the app will never run any cleanup steps (and with my recent change, won't run any shutdown steps either). Let me take a closer look and see if any of this makes sense.

[alexrudd2]

Thanks for taking a look. If you need anything from me, please let me know. (The CI runs in a fork too, if you'd prefer). That said, I don't know these tests and cannot comment on whether they're actually the right way to do things. You'll have to take it up with Jan.

[Dreamsorcerer]

So, I think if you just replace site.stop() with runner.cleanup(), as shown in the documentation: https://docs.aiohttp.org/en/stable/web_advanced.html#application-runners
at https://github.com/pymodbus-dev/pymodbus/blob/dev/pymodbus/server/simulator/http_server.py#L253

That should then work.

I think the specific problem, is that before that change site.stop() would call runner._server.shutdown() in order to shutdown all the connections/handlers. This ensures that the site has completely stopped and doesn't have any connections anymore, but it is also stopping all connections across the entire runner. So, if you had 2 sites and only wanted to stop 1 of them, you'd get the surprising behaviour of it killing all your connections in the other site as well.

For that reason, I moved that step to only happen in the runner.

[janiversen]

It would be better to do like HA does:

aiohttp==3.9.0b0;python_version>='3.12'
aiohttp==3.8.5;python_version<'3.12'

that works, without changing the code.