Be able to create asyncio modbusclients from within a coroutine

[Emilv2]

Based on PR #558 from @tiagocoutinho since that work seems to be stalled.

@memetb : I think calling get_event_loop() is the right call here, since the code still needs to work when no loop is present. The alternative of calling get_event_loop() would be something like:

try:
    loop = kwargs.get("loop") or asyncio.get_running_loop()
except RuntimeError:
    loop = asyncio.new_event_loop()

But that is exactly what get_event_loop() does.

There were some small style differences between tcp, udp, tls and serial, I made the the code more similar to make it easier to compare. I noticed that for tcp and tls asyncio.set_event_loop(loop) is called, but not for udp and serial, is that intended? I don't know enough of asyncio to say.

I don't think this way of also returning the loop and the client when running in a coroutine is particularly elegant since you can't await part of a returned tuple so you'll need to do something like this:

loop, client = AsyncModbusUDPClient(schedulers.ASYNC_IO, port=5020)
awaited_client = await client

I added some tests, but I expect they can be improved.

I also noticed that only the tls code would fail when calling get_event_loop() without catching RuntimeError, so the other code doesn't seem to be tested without a loop argument or a running loop.

[swamper123]

try:
    loop = kwargs.get("loop") or asyncio.get_running_loop()
except RuntimeError:
    loop = asyncio.new_event_loop()

But that is exactly what get_event_loop() does.

You are right about the "function" of get_event_loop() and I agree on using it, but only outside of coroutines.

The docs of 3.9 say:

Because this function has rather complex behavior (especially when custom event loop policies are in use), using the get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks.

I prefere skipping 3.6 anyway if we want to switch to Version 3.x in the future. 3.6 is a good base to built up, but there is still some ugly stuff to handle, which is not anymore in 3.7+ (where everything is a bit easier to handle over multiply versions).

[Emilv2]

What do you suggest replacing it with then?

[swamper123]

Well your snippet wasn't so bad. It would work the way as intended.

Like the docs say to get_event_loop():

Because this function has rather complex behavior (especially when custom event loop policies are in use), using the get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks.

I think replacing loop = asyncio.new_event_loop() with loop = asyncio.get_event_loop() is better, since we already checked if there is a coroutine running or not. In that case, the eventloop would be set on the current OS thread as well and you got everything you want.

If I remember it right, if we take your snippit again, you would need to execute set_event_loop(loop) as well, so the loop is running on the actual thread and not just a loop object is passed.

[Emilv2]

I think replacing loop = asyncio.new_event_loop() with loop = asyncio.get_event_loop() is better, since we already checked if there is a coroutine running or not. In that case, the eventloop would be set on the current OS thread as well and you got everything you want

I assume you meant to say replacing loop = asyncio.new_event_loop() with loop = asyncio.get_running_loop() is better? Simply replacing loop = asyncio.new_event_loop() with asyncio.get_running_loop() makes test/test_client_async.py::TestAsynchronousClient::testTlsAsyncioClient (TCP, UDP and serial are simply not tested, I assume they would fail too) fail with RuntimeError: no running event loop since no loop is provided and no loop is running.

[swamper123]

No no, I realy mean, what I wrote.

try:
    loop = kwargs.get("loop") or asyncio.get_running_loop()  # just checks if coroutine is running and get existing loop, if it exist. Otherwise raise RuntimeError
except RuntimeError:                                          # "if loop does not exist"
    loop = asyncio.get_event_loop()                          # create a new event loop and run it on the current OS Thread. 

This will give you always a legit loop without any problem. Then it doesn't matter if you are in a Coroutine or not!

I am not sure what kwargs.get("loop") does ... If I am uptodate with the topic, multiple loops shall not be possible in asyncio anymore...anyway.

[Emilv2]

Ok, I'll update the pull request. I don't really know about the kwargs.get("loop") either. According to the docs you can provide the loop, but if running multiple loops won't be possible anymore it seems indeed rendurant.

[Emilv2]

Updated the code as requested. I also changed kwargs.get(X , None) to kwargs.get(X) since None is already the default return value for get.

[sonarqubecloud]

SonarCloud Quality Gate failed.

4 Bugs
0 Vulnerabilities
2 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dhoomakethu]

@Emilv2 the best way to check is to run the example https://github.com/riptideio/pymodbus/blob/master/examples/common/async_asyncio_client.py against a server with all three conditions for the loop.

• Run with no loop
• Run with a loop that is not yet started
• Run with already running loop

[swamper123]

@Emilv2 ah...python 3.6 hasn't got the get_running_loop feature. You have to check the python version at that point. :/

[Emilv2]

Hmmm, I tested the 3 conditions (4 now with running in a coroutine) and there are still some small issues. The simplest would indeed be to drop both python <= 3.6 and the loop argument. Python 3.6 is eol by the end of the year anyway and as I understand it making the loop argument implicit and relying on get_running_loop() or get_event_loop() is recommended nowadays.

@dhoomakethu I think making an integration test of those examples would be a good idea, no?

[venkow]

@Emilv2 Hey, are you still planning on merging this change? Just wondering as it will be indeed quite useful ;)

[Emilv2]

Yes I'm still willing to work on it, but I'm not sure which python versions the maintainers want to support

[dhoomakethu]

@Emilv2 for 3.x python 3.6 3.7 and above is supported.

[Emilv2]

Ok. If only python 3.7 and above is supported, is it also the plan to drop the explicit loop argument or not? Dropping it would mean the code could be simplified a lot but would be a much bigger change to the code base (and documentation) than this pull request.

[dhoomakethu]

@Emilv2 I have kind picked your changes in to 3.0.0 branch already, I am yet to push the changes. JFYI

[Emilv2]

@dhoomakethu Ok great!

Right now my code doesn't work with all 4 conditions, I hope you fixed that if you keep the loop argument. It's a small change I didn't push yet.

[arjen-hiemstra]

@Emilv2 What is the status of this PR, are you still working on it? I just ran into this problem as well.

[Emilv2]

@arjen-hiemstra As I understood it @dhoomakethu was going to implement this himself. I can continue with it, but I will need to know from the maintainers where they want to go with the code (drop python 3.6? drop the loop argument?).

[dhoomakethu]

JFYI Python 3.6.x is going to be supported for some more time.

[Emilv2]

What about Python 3.10? It will deprecate the loop argument as I understand it correctly.

[swamper123]

JFYI Python 3.6.x is going to be supported for some more time.

Additional info to be more clear: 3.6 will be dropped 23rd Dec. 2021.

I would recommened to go on with 3.7, since the async stuff is more compatible with newer releases. 3.6 was revolutionary in that topic, but still had some parts not solved well.

[Emilv2]

Dropping Python <3.6 seems the most reasonable to me as well since I don't think pymodbus 3.0 is going to be released before then, or will it?

I checked the Python 3.10 docs, and for the low level api the loop argument will remain, so nothing has to be changed for Python 3.10. I think dropping the loop argument would simplify the code a lot, and from Python >3.7 it's not really necessary anymore. Or are there some special use cases for keeping the loop argument?

If everyone agrees on what has to be done I can give it a try since I have some time now.

[swamper123]

I hope you mean Dropping Python <=Python3.6 😉

I don't think pymodbus 3.0 is going to be released before then, or will it?

I guess this depends on the progress on the milestones (?).

I think dropping the loop argument would simplify the code a lot, and from Python >3.7 it's not really necessary anymore.

If I have it right in my mind, only one eventloop shall exist and that was the reason to drop the loop argument, since multiple loops can cause trouble. Whom who wants to use multiple loops, either doesn't understand asyncio or (hopefully) defitnitly knows what to do in multi/subprocessing enviroments...but that is such a special case, we should not think about.

[dhoomakethu]

@Emilv2 python3.6 forms the major version where pymodbus is being used and that is the only reason why I am planning to support it for some more time. In any case, if you are willing and have bandwidth please give it a try and let us know your findings.
@swamper123 asyncio is a beast on its own , I have see people use it in all weird ways but I agree we are not supposed to support every other odd case.

[track02]

I think I'm hitting this same issue as well when trying to setup an async client from a task inside an already running loop (i.e. passing in asyncio.get_running_loop() as the client loop argument) causes the factory to seemingly hang up

I did try modifying the library as outlined in another issue which solves the problem but I'm a bit wary of having to manage a local modified copy of the library, not sure if there's any other workaround at present?

More than happy to offer any help as it's a bit of a blocker for us the moment

[arjen-hiemstra]

@track02 My current work around is to circumvent the factory and create a client myself within my coroutine with:

from pymodbus.client.asynchronous.async_io import init_tcp_client

client_setup = await init_tcp_client(None, loop, host, port)
client = client_setup.protocol

rr = await client.read_holding_registers(48, 4)

Hope this helps

[Emilv2]

I see that in pymodbus/client/asynchronous/async_io/__init__.py there is also another check for a provided loop self.loop = loop or asyncio.get_event_loop(). It seems to me that this should be done in only one place, no?

[track02]

@arjen-hiemstra - Thank you, that's a lot simpler and so far so good after a little testing today

@Emilv2 - I'm still picking through how things fit together so apologies if I have missed something
Seems a bit unnecessary as the asyncio factory -> init_tcp_client already look to do the same check to figure out the loop to use, so maybe loop can be a non-optional arg

Or consider changing get_event_loop() to get_running_loop() maybe just me but I'm unsure if a client should be setting up a loop (if not found) over being provided one to use?

[dhoomakethu]

All changes merged to 3.0.0 branch refer #658