async datastore in modbus server #2127

ilkka-ollakka · 2024-03-28T11:38:41Z

Changed datastore to be used by async-flow by modbus server, allowing datastores to be async related to getValues/setValues use.

janiversen

The idea of making datastore async is something on our wish list, but I see multiple problems with your PR:

Client code is changed (datastore is solely for server)
Async are added in several places, breaking the sync client
There are far too many files changed for a simple datastore change.

I think a much simpler approach would be to add 2 new function in datastore:

async_getValues() and async_setValues
That way most of your changes outside datastore is not needed.
The 2 new functions can simply call the old getValues() / setValues()

Apart from that the CI is pretty red, something that needs to be solved.

pymodbus/bit_read_message.py

janiversen · 2024-03-28T12:34:40Z

pymodbus/client/base.py

        returns number of bytes consumed
        """
-        self.framer.processIncomingPacket(data, self._handle_response, slave=0)
+        task = asyncio.create_task(


This is client code and have nothing to do with the datastore which is purely server.

Ahh I see that CI is now Green, one important step done.

You still need to limit your changes, especially so that NO client code is included.

But let me repeat, the idea of the PR is good and very welcome.

I think I checked that sync client still works as expected, but I agree that change is quite wide.

I'll take another round to check more limited changes and try to reduce touching the client-used code changes.

Thanks for the quick comments.

I am pretty sure, that adding 2 new async methods(), will meet your requirements, and limit all changes to the datastore directory + test.

request me as reviewer when you are ready.

I think the new round is closer to your proposal on approach and now it should be contained only on serverside and datastore.

janiversen · 2024-03-28T12:55:18Z

Also once the implementation is OK, you need to add test cases, that show the new code works.

ilkka-ollakka · 2024-03-30T12:24:28Z

pymodbus/datastore/remote.py

        return True

-    def getValues(self, fc_as_hex, _address, _count=1):
+    async def async_getValues(self, fc_as_hex, address, count=1):


I left these changes as separate commit, incase there is some desire to keep this still intact.

it is the other way around, we do not want the cascade changes your original PR contained and still contains.

Having the async_getValues, should solve your problem, so please revert all the cascaded changes in the system (your PR still touches 31 files).

janiversen

review note:

Please remove all the cascaded changes, they are not needed, if you do not change the original methods.

ilkka-ollakka · 2024-03-30T17:46:39Z

I reduced the execute change to be only related the ones that call get/setvalues and server to check if it needs to await the request.

default port 8080 can be in use when running test in developer maching, so better to use port that is available

by default those just call self.set/getValues but can be used to implement more async datastore

update tests and callback code in server-code and messages

messages and serverside defaults to async-calls now and async default is to call the sync version of itself, allowing to build async datastorage functionality

janiversen · 2024-03-30T18:45:16Z

You are still touching 19 files, far too many !

You did not do, what I asked you to do, to make 2 new methods, you renamed the old ones and that causes the cascade effect.

The server code is not ready for an async datastore, until we have removed the sync parts around it. There are a lot more problems than just making the methods async.

However having async methods allowing the datastore to be accessed outside is not a problem.

janiversen

Seems you are trying to make unrelated changed in the same PR, and it seems you have not thought about how the sync client works.

janiversen · 2024-03-30T18:46:45Z

pymodbus/bit_read_message.py

        ReadBitsRequestBase.__init__(self, address, count, slave, **kwargs)

-    def execute(self, context):
+    async def execute(self, context):


This is also called from the SYNC client.

janiversen · 2024-03-30T18:47:33Z

pymodbus/bit_read_message.py

        if not context.validate(self.function_code, self.address, self.count):
            return self.doException(merror.IllegalAddress)
-        values = context.getValues(self.function_code, self.address, self.count)
+        values = await context.async_getValues(


This too is not exclusive for the server.

janiversen · 2024-03-30T18:49:04Z

pymodbus/datastore/context.py

+        :param count: The number of values to retrieve
+        :returns: The requested values from a:a+c
+        """
+        del fc_as_hex, address, count


Why delete the variables, that does not make a lot of sense.

they are not used, so makes it more clear in that point. Mypy and ruff etc easily complain on those. But I agree it is more of taste and not really critical to be in my point of view.

use "_" that is the standard way to tell a variable is unused.

It might not be critical from your POW, but from me as maintainer it is negative.

sure, I can change it to be with "_".

janiversen · 2024-03-30T18:49:13Z

pymodbus/datastore/context.py

+        :param address: The starting address
+        :param values: The new values to be set
+        """
+        del fc_as_hex, address, values


janiversen · 2024-03-30T18:51:28Z

pymodbus/datastore/remote.py

+        kwargs = {}
+        if self.slave:
+            kwargs["slave"] = self.slave
+        self.result = await getattr(self._client, func_fc)(address, count, **kwargs)


This looks wrong to me, the code is designed so we do not need to call getattr, which is very expensive function.

janiversen · 2024-03-30T18:53:37Z

pymodbus/datastore/remote.py

-            "i16": lambda a, v: self._client.write_registers(
-                a, v, **kwargs
-            ),
+            "d5": "write_coil",


This is not the same...but might be the reason I saw a getattr earlier.

We do not want that, apart from that what does it have to do with getValues being async ?

this is mainly due lamdas and await don't work together. It can be redone without getattr use most likely easily.

janiversen · 2024-03-30T19:01:01Z

your description says:
"Changes codepaths to be async to allow datastore getValues/setValues to be async methods.!"

My suggestion have constantly been, add 2 new methods instead, that will allow you to have async access from outside the server.

You do not write it, but if your purpose is to write a new datastore, you are in for a far bigger job, because the server shares mixin, execute, transaction and framer code with the client both sync and async. So in order to make the datastore async internally, you need to change the server to not share code with any sync part (which includes a couple of examples). This is something that should NOT be done in a single PR, but probably at least 10 PRs

ilkka-ollakka · 2024-03-31T11:38:14Z

your description says: "Changes codepaths to be async to allow datastore getValues/setValues to be async methods.!"

That was my original intent, to get async datastore to server. Not really to have async access from outside to datastore.

Sorry for causing frustration with my miscommunication on original description and intent related to this PR. I understand that it must have been frustrating.

You do not write it, but if your purpose is to write a new datastore, you are in for a far bigger job, because the server shares mixin, execute, transaction and framer code with the client both sync and async.
So in order to make the datastore async internally, you need to change the server to not share code with any sync part (which includes a couple of examples). This is something that should NOT be done in a single PR, but probably at least 10 PRs

From those I think only execute is actually one that blocks calling datastore as async way from server?

To be more splitted, would it be suitable approach for example to change ServerDecoder so that it returns AsyncReadCoilsRequest instead of ReadCoilsRequest etc, and those Async ones would be inherited from the original but have async execute? This way sync flow would be not touched, but it allows to take step closer to async datastore in server and work on that side without needing to touch sync execute-path?

I understand that I might not be enough, so more of mapping on things that are lacking and increasing my own understanding of the scope.

janiversen · 2024-03-31T12:43:14Z

From those I think only execute is actually one that blocks calling datastore as async way from server?

That is basically correct, but execute is also used in the client code.

To be more splitted, would it be suitable approach for example to change ServerDecoder so that it returns AsyncReadCoilsRequest instead of ReadCoilsRequest etc, and those Async ones would be inherited from the original but have async execute? This way sync flow would be not touched, but it allows to take step closer to async datastore in server and work on that side without needing to touch sync execute-path?

No we do not want to double that amount of Request/Response classes. It is clear that execute should be async when used in asyncio, so maybe a async_execute in a message base could do the trix.....this is just a thought nothing more.

I understand that I might not be enough, so more of mapping on things that are lacking and increasing my own understanding of the scope.

The easiest path, is to look at asyncio.py, because that is the whole server. You basically need to "pursuade" that file to call async_getValues instead of getValues. That is the best way (if possible), second best is to have an async_execute in 1 message base class.

But please no duplication of code and no cascading changes....allowing that is calling for trouble in future maintenance.

I think it is doable, I looked at it in depth about a year ago, when we wanted to make a better forwarder, but got stopped by more urgent matters.

ilkka-ollakka · 2024-03-31T13:01:34Z

No we do not want to double that amount of Request/Response classes. It is clear that execute should be async when used in asyncio, so maybe a async_execute in a message base could do the trix.....this is just a thought nothing more.

Ok, that sounds good starting point to recheck things.

The easiest path, is to look at asyncio.py, because that is the whole server. You basically need to "pursuade" that file to call async_getValues instead of getValues. That is the best way (if possible), second best is to have an async_execute in 1 message base class.

Currently it does call execute and doesn't know anything about getValues/setValues, but it could be easily changed to call that async_execute. But that would cause change in some Request-classes where there is calls to getValues/setValues.

But please no duplication of code and no cascading changes....allowing that is calling for trouble in future maintenance.

I'll check if I can do something to keep async_execute and execute with minimal code duplication.

janiversen · 2024-03-31T13:16:59Z

You might need to look from where the execute is called, if it turns out to be only asyncio.py then things are simpler, but I suspect at least some axamples uses the execute.

janiversen · 2024-03-31T13:28:31Z

A short look at the code, seems to suggest that the execute in the message classes is only used in asyncio.py (it is really a wrong name if that is the case).

If this is the case, the better approach would be to have:

first PR, that changes the execute in asyncio to be async (hopefully without any side effects)
second PR, that changes the execute in all message classes to be async (This is a massive change, so it should ONLY contain edits, no functional changes)
third PR, that changes the datastore.

If you do it all in one PR, it is practically impossible to review, and if/when problems occur down the line, it is very difficult to pin point the cause.

ilkka-ollakka · 2024-03-31T15:21:16Z

I'll split the things to 4 PR's and link those here.

ilkka-ollakka · 2024-04-04T10:51:59Z

Intent of this PR is splitted to separated PR's and linked here, so closing this one as it has ran it course

ilkka-ollakka force-pushed the feat/async_pymodbus branch 3 times, most recently from 46e1932 to ecbdd74 Compare March 28, 2024 12:39

janiversen requested changes Mar 28, 2024

View reviewed changes

ilkka-ollakka force-pushed the feat/async_pymodbus branch 2 times, most recently from 88ff71e to 9a193ad Compare March 30, 2024 12:07

ilkka-ollakka requested a review from janiversen March 30, 2024 12:07

ilkka-ollakka commented Mar 30, 2024

View reviewed changes

janiversen requested changes Mar 30, 2024

View reviewed changes

ilkka-ollakka force-pushed the feat/async_pymodbus branch from 9a193ad to 030439b Compare March 30, 2024 17:44

ilkka-ollakka requested a review from janiversen March 30, 2024 17:45

ilkka-ollakka added 5 commits March 30, 2024 19:47

test_simulator: use unused_tcp_port fixture

412cb10

default port 8080 can be in use when running test in developer maching, so better to use port that is available

datastore: add async_setValues/getValues methods

8f437d2

by default those just call self.set/getValues but can be used to implement more async datastore

server: change datastore related request execute to be async

bf85de9

update tests and callback code in server-code and messages

datastore: change code to call async_getValues/setValues

bafee10

messages and serverside defaults to async-calls now and async default is to call the sync version of itself, allowing to build async datastorage functionality

datastore: change remote_datastore to be async

ca7bbeb

ilkka-ollakka force-pushed the feat/async_pymodbus branch from 030439b to ca7bbeb Compare March 30, 2024 17:49

janiversen requested changes Mar 30, 2024

View reviewed changes

ilkka-ollakka changed the title ~~async datastore/context~~ async datastore in modbus server Mar 31, 2024

ilkka-ollakka mentioned this pull request Mar 31, 2024

modbus_server: execute to handle message execute to be coroutine or method #2139

Merged

ilkka-ollakka mentioned this pull request Apr 2, 2024

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

Merged

ilkka-ollakka marked this pull request as draft April 2, 2024 08:03

This was referenced Apr 2, 2024

datastore: add async_setValues/getValues methods #2143

Closed

call async datastore from modbus server execute flow #2144

Merged

ilkka-ollakka closed this Apr 4, 2024

async datastore in modbus server #2127

async datastore in modbus server #2127

Uh oh!

Conversation

ilkka-ollakka commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janiversen commented Mar 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

ilkka-ollakka commented Mar 30, 2024

Uh oh!

janiversen commented Mar 30, 2024

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janiversen commented Mar 30, 2024

Uh oh!

ilkka-ollakka commented Mar 31, 2024

Uh oh!

janiversen commented Mar 31, 2024

Uh oh!

ilkka-ollakka commented Mar 31, 2024

Uh oh!

janiversen commented Mar 31, 2024

Uh oh!

janiversen commented Mar 31, 2024

Uh oh!

ilkka-ollakka commented Mar 31, 2024

Uh oh!

ilkka-ollakka commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ilkka-ollakka commented Mar 28, 2024 •

edited

Loading