search

pymodbus-dev / pymodbus

A full modbus protocol written in python
Other
2.16k stars 889 forks source link

Request/Response: change execute to be async method #2142

Closed ilkka-ollakka closed 2 months ago

ilkka-ollakka commented 3 months ago

contains only async/await edits, no functional changes otherwise. This combined with https://github.com/pymodbus-dev/pymodbus/pull/2144 allows to utilize async datastore calls in modbus server

Split from https://github.com/pymodbus-dev/pymodbus/pull/2127 and depends on https://github.com/pymodbus-dev/pymodbus/pull/2139

ilkka-ollakka commented 3 months ago

In theory execute don't need to be async, but some of them are checking results of getValues/setValues, so those should be reworked to be out from execute in that case. I had the impression that only server-code called execute and this was ok approach. But I'll take second look of the comments and check what I missed.

janiversen commented 3 months ago

Did you look at how we define the T which is the return from the mixin methods, that secures you can use the modhid as sync/async and satisfy mypy.

janiversen commented 3 months ago

you might be right that the execute in the different messages is only called from asyncio (and a couple of special examples), however I am afraid of the side effects, especially since you gain absolutely nothing by just declaring the execute async, and the very least you would need to call an async method in the datastore.

janiversen commented 3 months ago

I will take a closer look, once you have a green CI.

ilkka-ollakka commented 3 months ago

you might be right that the execute in the different messages is only called from asyncio (and a couple of special examples), however I am afraid of the side effects, especially since you gain absolutely nothing by just declaring the execute async, and the very least you would need to call an async method in the datastore.

Yes, My initial idea was to do that change in separate PR, that I haven't yet opened. Just to make this one only async/await changes and in that point of view, simpler to review. Referring to https://github.com/pymodbus-dev/pymodbus/pull/2127#issuecomment-2028737549

So plan is to open separate PR that will add async_get/setValues to datastore and changes execute to use those with await, then fourth to change remote-datastore to use client as async flow.

ilkka-ollakka commented 3 months ago

you might be right that the execute in the different messages is only called from asyncio (and a couple of special examples), however I am afraid of the side effects, especially since you gain absolutely nothing by just declaring the execute async, and the very least you would need to call an async method in the datastore.

I'm also fine to leave this PR open until I have opened the remaining PR's, so you can check those aswell to see end goal better?

ilkka-ollakka commented 3 months ago

And btw it seems you have not analyzed the code. The execute is both sync and async (as in it returns a future). Execute splits at a lower level to async_execute when called in an async environment

Isn't that only in ModbusClient.execute codepath and not in server-codepath used ModbusRequest.execute ?

janiversen commented 3 months ago

Sorry you are right.

janiversen commented 3 months ago

I might not had said it correctly, but you need to attach this bottom-up, meaning e.g. this PR does not make sense until you have async datastore in place.

I review each PR as a closed part (which of course can form part of bigger picture), and each PR must be understandable and have a reason (it is not a reason, to state this is needed for a future change somewhere else). This PR adds "async" to a method that have no awaits, so it does not make sense.

I hope I have explained myself better....I am all in favor of having an async datastore, I just need to ensure that the code and the architecture do not break.

ilkka-ollakka commented 3 months ago

I might not had said it correctly, but you need to attach this bottom-up, meaning e.g. this PR does not make sense until you have async datastore in place.

That makes sense

I review each PR as a closed part (which of course can form part of bigger picture), and each PR must be understandable and have a reason (it is not a reason, to state this is needed for a future change somewhere else). This PR adds "async" to a method that have no awaits, so it does not make sense.

Understandable

I hope I have explained myself better....I am all in favor of having an async datastore, I just need to ensure that the code and the architecture do not break.

Yes, I'll try to get the remaining PR's open so things can be justified and revied easier.

janiversen commented 3 months ago

btw just for information, you might have noticed that asyncio.py calls self.framer.processIncomingPacket which is not async (since it is shared with the sync client) and thus giving you problems. We are currently working a lot to refactor framer, and once complete it will be totally async.

In the meantime, you can replace the "lambda" with functols.partial calling the async executor, with async_execute as argument. This is not a needed change, but just to show you how to get around having 2 execute methods.

ilkka-ollakka commented 2 months ago

You change all server execute to be async, but execute in asyncio is still sync ?

Yes, as execute is passed to callback for processIncomingPacket, and I wanted to scope down the changes as much as possible, I could have used partial there and remove the lambda, but I saw it as not required change at the moment in this scope.

janiversen commented 2 months ago

Closing this since it changed to several small PR

ilkka-ollakka commented 2 months ago

Closing this since it changed to several small PR

Hi, I think you are mixing https://github.com/pymodbus-dev/pymodbus/pull/2127 and this PR, as this PR was split out from the bigger PR, which is 2127 and if you look the change of this PR, it is one of the small changes

janiversen commented 2 months ago

Sounds correct, sorry for the mistake.

janiversen commented 2 months ago

It's still draft though.

ilkka-ollakka commented 2 months ago

It's still draft though.

I rebased and re-opened the PR

ilkka-ollakka commented 2 months ago

windows CI seems to fail on not having python

ilkka-ollakka commented 2 months ago

The changes as such are OK, however I miss an await in asyncio.py where execute is called.... it seems to me that this PR guarantees that execute delivers a future.

That is already in, and was merged in https://github.com/pymodbus-dev/pymodbus/pull/2139

janiversen commented 2 months ago

As far as I can see it is not changed.

_async_execute() still contains the check iscoroutine(), which is no longer needed.

ilkka-ollakka commented 2 months ago

As far as I can see it is not changed.

_async_execute() still contains the check iscoroutine(), which is no longer needed.

I'll add commit to remove it