Request/Response: change execute to be async method

[ilkka-ollakka]

contains only async/await edits, no functional changes otherwise. This combined with #2144 allows to utilize async datastore calls in modbus server

Split from #2127 and depends on #2139

[janiversen]

This is wrong and will not work for the sync client.

I thought you were working on the changes in asyncio in another PR, where you demonstrated that execute do not need to be async.

[ilkka-ollakka]

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]

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

[janiversen]

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]

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]

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

[ilkka-ollakka]

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 #2127 (comment)

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]

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]

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]

Sorry you are right.

[janiversen]

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]

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]

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.

[janiversen]

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

[janiversen]

This seems wrong ! the client should be affected by a server change.

[ilkka-ollakka]

that is RequestExecute, so for example ReadCoilsRequest, and as that mocks server-responses, it needs to do that async execute call.

[janiversen]

You are correct, thanks for pointing it out.

[ilkka-ollakka]

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]

Closing this since it changed to several small PR

[ilkka-ollakka]

Closing this since it changed to several small PR

Hi, I think you are mixing #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]

Sounds correct, sorry for the mistake.

[janiversen]

It's still draft though.

[ilkka-ollakka]

It's still draft though.

I rebased and re-opened the PR

[ilkka-ollakka]

windows CI seems to fail on not having python

[janiversen]

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.

[ilkka-ollakka]

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 #2139

[janiversen]

As far as I can see it is not changed.

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

[ilkka-ollakka]

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

[janiversen]

LGTM, thanks.