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

ilkka-ollakka · 2024-03-31T14:23:00Z

Split from #2127

execute() is used as callback, so we don't change it to async, but we use it to create async_task that can handle async calls

janiversen

Good PR, the only real problem is the create_task, which we really do not want for performance reasons.

janiversen · 2024-03-31T14:29:03Z

pymodbus/server/async_io.py

@@ -173,17 +175,28 @@ def execute(self, request, *addr):
        if self.server.request_tracer:
            self.server.request_tracer(request, *addr)

+        task = self.loop.create_task(self._async_execute(request, *addr))


This is a lot of overhead, creating a task for every request. It is possible and efficient to call async methods from sync methods.

Seems that asyncio documentation seems to prefer create_task, but if you have proposal to alternative approach, I can change it.

I changed this to run_coroutine_threadsafe, ensure_future seemed to end up using create_task.

janiversen · 2024-03-31T14:30:35Z

pymodbus/server/async_io.py

@@ -49,6 +49,8 @@ def __init__(self, owner):
        self.receive_queue: asyncio.Queue = asyncio.Queue()
        self.handler_task = None  # coroutine to be run on asyncio loop
        self.framer: ModbusFramer
+        self.request_tasks: set[asyncio.Task] = set()


This is opening for parallel processing in the server, that is one big can of worms, please do not open that.

I'll remove use of set

janiversen · 2024-03-31T14:31:42Z

pymodbus/server/async_io.py

        try:
            if self.server.broadcast_enable and not request.slave_id:
                broadcast = True
                # if broadcasting then execute on all slave contexts,
                # note response will be ignored
                for slave_id in self.server.context.slaves():
                    response = request.execute(self.server.context[slave_id])
+                    if asyncio.iscoroutine(response):


A short comment telling this is temporary would be nice.

I'll add mention about temporary

janiversen · 2024-03-31T14:33:42Z

pymodbus/server/async_io.py

            else:
                context = self.server.context[request.slave_id]
                response = request.execute(context)
+                if asyncio.iscoroutine(response):


If you move the "if" outside the "else:" you do not need to duplicate the test.

Remark: the "for slave_id" is not the best programming, because it can only be there once.

Ah, good point, I'll move it outside of else block.

Only problem with it being outside, is that with broadcast it doesn't raise exception in other than last request. But as this being temporary code, I think it is acceptable transition phase?

I left these in place, mainly so that it still raises exceptions from execute in broadcast when it happens and it would end up leaving other execute calls to be not awaited.

janiversen · 2024-03-31T14:34:42Z

pymodbus/server/async_io.py

        # no response when broadcasting
-        if not broadcast:
+        if not broadcast and response is not None:


Why can response be None now, when it wasn't before ?

I set it to None at the beginning, I'll clear it out, so it is closer to original code.

janiversen

One very small change and this seems ready to go.

janiversen · 2024-04-02T07:08:59Z

pymodbus/server/async_io.py

@@ -49,6 +49,7 @@ def __init__(self, owner):
        self.receive_queue: asyncio.Queue = asyncio.Queue()
        self.handler_task = None  # coroutine to be run on asyncio loop
        self.framer: ModbusFramer
+        self.event_loop = asyncio.get_running_loop()


Please call the variable self.loop to be consistent with the rest of the code.

…nes or normal methods

janiversen

LGTM, thanks.

…nes or normal methods (#2139)

* add _legacy_decoder to message rtu (#2119) * Add generate_ssl() to TLS client as helper. (#2120) * ASCII framer using message decode() (#2128) * SOCKET/TLS framer using message decode(). (#2129) * Fix decode for wrong mdap len. * Streamline message class. (#2133) * modbus_server: call execute in a way that those can be either coroutines or normal methods (#2139) * Clean datastore setValues. (#2145) * fixed kwargs not being expanded for actions on bit registers, adjusted tests to catch this issue (#2161) * datastore: add async_setValues/getValues methods (#2165) Co-authored-by: Ilkka Ollakka <[email protected]> * Request/Response: change execute to be async method (#2142) * Bump actions CI. (#2166) * Fix usage of AsyncModbusTcpClient in client docs page (#2169) * Sphinx: do not turn warnings into errors. * Add minimal devcontainer. (#2172) * Transaction id overrun. * call async datastore from modbus server (#2144) * Datastore will not return ExceptionResponse. (#2175) * Describe zero_mode in ModbusSlaveContext.__init__ (#2187) * Solve pylint error. * Show error if example is run without support files. (#2189) * Fix usage file names (#2194) * Update client.rst (#2199) * Transaction_id for serial == 0. (#2208) * Remember to remove serial writer. (#2209) * Fix writing to serial (rs485) on windows os. (#2191) Co-authored-by: jan iversen <[email protected]> * test convert registers with 1234.... (#2217) * Solve serial unrequested frame. (#2219) * Log comm retries. (#2220) * prepare v3.6.9. * pylint. * Remove python 3.8 from CI. --------- Co-authored-by: Ilkka Ollakka <[email protected]> Co-authored-by: sumguytho <[email protected]> Co-authored-by: Ilkka Ollakka <[email protected]> Co-authored-by: Yohrog <[email protected]> Co-authored-by: James Cameron <[email protected]> Co-authored-by: Qi Li <[email protected]> Co-authored-by: andrew-harness <[email protected]>

janiversen requested changes Mar 31, 2024

View reviewed changes

ilkka-ollakka force-pushed the feat/async_io_execute branch from 170fa30 to 3ce922a Compare April 2, 2024 05:33

ilkka-ollakka mentioned this pull request Apr 2, 2024

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

Merged

ilkka-ollakka requested a review from janiversen April 2, 2024 05:42

janiversen approved these changes Apr 2, 2024

View reviewed changes

modbus_server: call execute in a way that those can be either corouti…

6ffb46f

…nes or normal methods

ilkka-ollakka force-pushed the feat/async_io_execute branch from 3ce922a to 6ffb46f Compare April 2, 2024 07:41

janiversen approved these changes Apr 2, 2024

View reviewed changes

janiversen merged commit 24c200c into pymodbus-dev:dev Apr 2, 2024
1 check passed

janiversen pushed a commit that referenced this pull request Jun 18, 2024

modbus_server: call execute in a way that those can be either corouti…

0f12581

…nes or normal methods (#2139)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ilkka-ollakka commented Mar 31, 2024 •

edited

Loading

janiversen left a comment

janiversen Mar 31, 2024

ilkka-ollakka Apr 1, 2024

ilkka-ollakka Apr 2, 2024

janiversen Mar 31, 2024

ilkka-ollakka Apr 1, 2024

janiversen Mar 31, 2024

ilkka-ollakka Apr 1, 2024

janiversen Mar 31, 2024

ilkka-ollakka Apr 1, 2024

ilkka-ollakka Apr 1, 2024

ilkka-ollakka Apr 2, 2024

janiversen Mar 31, 2024

ilkka-ollakka Apr 1, 2024

janiversen left a comment

janiversen Apr 2, 2024

ilkka-ollakka Apr 2, 2024

janiversen left a comment

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

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

Conversation

ilkka-ollakka commented Mar 31, 2024 • edited Loading

janiversen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen left a comment

Choose a reason for hiding this comment

ilkka-ollakka commented Mar 31, 2024 •

edited

Loading