[sankalpsthakur]

When an async execute() is cancelled while it waits, the device can still answer after the next request is sent. With RTU/ASCII, which have no transaction id, and the same function code, the next request to that device then returns that stale data, as in #3070.

• On cancel, execute() keeps a fresh response future for the late response, and the next execute() to the same device with the same function code waits for it before sending, at most timeout_connect after the cancel. Requests to other devices, with another function code, or with a transaction id (socket framers, where handleFrame skips an old transaction id) are not delayed.
• callback_data skips a response whose function code differs from the request's, before it sets the response future. Test wrong response gets discarded. #3078 drops it in execute(), after it has taken the future, so a correct response right behind it was ignored and the request was sent again.
• CancelledError still propagates as in Let CancelledError propagate from async execute #3010, and there is no API change.

Validation

• The cancel scenarios from Test case for cancelling request. #3077 now run (skip and pragma: no cover removed): timeout, resend, resend_response, new_id and new_fc. On dev (45c8933), resend, resend_response and new_fc fail; with this change all pass.
• test_transaction_execute_cancel_no_wait (new): after a cancel, a request to another device, a request with another function code (FramerRTU) or a request with a transaction id (FramerSocket) is sent at once and gets its own response. The function code case fails on dev.
• pytest test/transaction: 54 passed on Python 3.10.18 and 3.14.7 (macOS), with 100% line and branch coverage of pymodbus/transaction and test/transaction.
• ruff check, ruff format --check, codespell and zuban check on the touched files are clean, and pylint reports nothing new. execute() was already at the C901 limit of 10, so it gets # noqa: C901, like convert_from_registers.

Fixes #3070

AI disclosure: written with an AI coding agent and tested as described above.

[janiversen]

I just tried your test case, as expected it shows the problem, so with that I will try

• revert the 2 PRs that have changed cancelation, and ensure the test passes.
• if so, then fix the PRs.

I do suspect however that the problem might be older, because I can produce the problem without cancelling:

1. App sends a request with timeout.
2. No response is received before timeout, and app receives a ModbusIOException.
3. App sends new request
4. Device responds to first request, which are returned as response to second request
5. Device responds to second request, which are discarded

This is of course a configuration problem (timeout too small), but the effect seems identical to cancelling.

I will keep you informed of my findings, because your method to solve might be the best one.

[janiversen]

Used your test case on v3.15.0

modified line 321:

   with pytest.raises(ModbusIOException):

to adapt to the non passing of cancel exception.

Your test FAILS, meaning this is as suspected a very old problem.

Your fix raises a big problem, because it should be allowed to send a request to another device on the same line, without delay:

Ex 1:

• App sends request1, dev_id 1
• App cancels request1
• App sends request2, dev_id 2
• receive response1 (discarded because dev_id not matching)
• receive response2, passed to app

Ex 2:

• App sends request1, dev_id 1
• App cancels request1
• App sends request2, dev_id 2
• receive response2, passed to app
• App sends request1, dev_id 1
• wait for timeout
• receive response1 within wait time (discarded because no request outstanding)
• receive response2, passed to app

If we get those 2 examples working, the only outstanding problem is if timeout is configured too low:

• App sends request1, dev_id 1
• App cancels request1
• App sends request2, dev_id 1
• wait for timeout
• receive response1, after timeout, passed to app as response2

This seems theoretically insolvable, but it is a configuration problem.

[janiversen]

I have updated your test case in #3077, to cover the scenarios I think are important.

Please have a look at it, and feel free to amend commits if something seems wrong.

As you can see, it causes a bit of problems with your PR; however I think you are on the right road.

And of course you are added as co-author, since the original comes from your PR.

[sankalpsthakur]

thanks for writing the scenarios. i ran your test from #3077 here. _cancel_send waits for write.call_count < 2, so its second call (the third write, in new_id and new_fc) returns before the request is sent and the response goes in first, and in resend the request is started twice. with only these two changes to the harness:

     async def _cancel_send(self, transact, transport, delay_cond, pdu):
         """Create task and wait."""
+        writes = transport.write.call_count
         resp = asyncio.create_task(transact.execute(False, pdu))
         delay = 0
-        while transport.write.call_count < 2:
+        while transport.write.call_count == writes:
             await asyncio.sleep(0.1)
             delay += 1
@@
             case "resend":  # resend within timeout, must delay until after timeout
-                resp = asyncio.create_task(transact.execute(False, req_org2))
                 await asyncio.sleep(0.1)

timeout and new_id pass on dev, on ce90cff and on bbf30db, which i just pushed: after a cancel only the next request to the same device waits, and requests to other devices and requests with a transaction id go out at once. (new_id also passes with the old global wait, because with timeout 0.5 the wait ends within 5 ticks. test_transaction_execute_cancel_no_wait in this PR uses 5 s and checks that the request to device 2 goes out at once.) resend and new_fc fail on all three, and those two are your call:

1. resend: the late response arrives before the resend, the cancelled request's future takes it, and the resend goes out at once (assert 1 > 5). should the resend wait until the cancel time + timeout_connect even then? with timeout 0.5 the resend starts about 0.2 s after the cancel, so that wait would be about 0.3 s, and delay > 5 can't hold either way.
2. new_fc: the late holding register response arrives while the input register request is outstanding, and the function code check from Partial solve ext. cancel sideeffects. #3072 raises "Unsolicited response received". your comment says a request with another function code goes out without delay, and for the test to pass, a response with another function code has to be dropped instead of raising. should i change it that way? and the assert expects resp_org.registers, did you mean resp_fc.registers?

and how do you want to land it: rebase this PR on #3077, or take your test into this one? both change test_transaction_execute_cancel.

[janiversen]

I am happy you liked the scenarios, it was easier than writing pseudo code :-)

I have added your 2 first suggestions, thanks they are very much correct, and I have merged the PR to dev.

Your 3 suggestion (resend) is not added, since it is a resend WITHIN the timeout, actually this is the most likely scenario....app cancels the request, and immediately reissued the same request.

If you saw something I did not see in this scenario, please add your change again, but please explain why mine is wrong.

resend:

I am happy with the delay being timeout_connect, not considering cancel time, whatever is easier for you,

new_fc:

I think I just changed that on dev. Sorry it was a mix of 2 places making the same check. the message should be dropped.

I think I made a wrong assert, feel free to correct it.

Please rebase your PR on dev, and remove "pragma: no cover", "mark.skip" in transaction.py in your pr.

Thanks a lot for your work, this is NOT an easy task and you are doing a great job !!!! I really hope you would like to help with other interesting problems later.

[janiversen]

Sorry the conflict is because I added a test to check that wrong responses are discarded.

[sankalpsthakur]

thanks, glad the scenarios helped, and thanks for the kind words. i rebased this PR on dev and removed the skip and the pragma: no cover lines (except the one on case _:). 0595695 has the fix, 03343a6 the changes to your test. test/transaction passes (54 tests, 100% coverage) on python 3.10 and 3.14.

new_fc: dev drops the response in execute(), but only after resp_org has set the future. resp_fc arrives before execute() runs again and is ignored ("received pdu without a corresponding request, IGNORING"), and the request sent again after "Skipping" gets no response, so new_fc ends with "No response received after 0 retries". so this PR checks the function code in callback_data, before the future is set. after a cancel, only the next request to that device with the same function code waits, so the input register request goes out at once. i changed the assert to resp_fc.registers.

resend: your scenario is right, the app cancels and sends the same request again at once. but the test had two requests: the create_task you kept is the resend, and _cancel_send then starts a second one. _cancel_send measures the next write, which is the resend's, and the resend goes out as soon as resp_org arrives (1 tick), because the cancelled request takes resp_org. the second request then gets no response and fails with "No response received after 0 retries". and the resend starts 0.1 s after the cancel and waits at most until the cancel time + timeout_connect, about 4 ticks (a full timeout_connect from the resend would be 5), so delay > 5 can't hold with timeout 0.5. so it is now two cases:

• resend: send again at once, no late response: it waits until the timeout, delay > 3.
• resend_response: your sequence, with the second request replaced by checks of transport.write.call_count: the resend isn't sent until resp_org arrives, then goes out at once and gets resp_org2.

the delay keeps the cancel time, since that was already in place. i also removed the guard and its pragma in test_transaction_execute scenario 9; the guard is always true there.

i'd be glad to help with other problems later too.

[janiversen]

The resend was my bad, thanks for catching it.

I will review your code a bit later.

[janiversen]

LGTM, nice idea with the dict key as tuple.

[janiversen]

Would be nice with a followup PR that got rid of the noqa, by moving a part out in a new method

[sankalpsthakur]

thanks for merging, and for writing the scenarios. happy to take the next one: is there a problem you'd like me to look at?

[janiversen]

For a starter please submit a PR, making the noqa unnecessary

    async def execute(  # noqa: C901

We try to solve the issues/problems when they arrives, so there are no open problems, but there are a number of pending enhancements:

• Make a simulator UI in Typescript, that interacts with the server (a very rudimentary UI exist)
• Once the new PDU structure is released, the server could be refactored, combine with the simulator UI.
• Make the sync client use async execute internally, saving a lot of duplicated code.

These are all bigger problems, but can of course be split in chunks and thus something easier to make and to review.

[sankalpsthakur]

For a starter please submit a PR, making the noqa unnecessary

    async def execute(  # noqa: C901

We try to solve the issues/problems when they arrives, so there are no open problems, but there are a number of pending enhancements:

• Make a simulator UI in Typescript, that interacts with the server (a very rudimentary UI exist)

• Once the new PDU structure is released, the server could be refactored, combine with the simulator UI.

• Make the sync client use async execute internally, saving a lot of duplicated code.

These are all bigger problems, but can of course be split in chunks and thus something easier to make and to review.

understood

[sankalpsthakur]

Thanks for merging #3084. I'd like to take the third item, the sync client using async execute internally.

My plan: each sync client owns its async counterpart and a private event loop running in a daemon thread. A sync call submits the coroutine with asyncio.run_coroutine_threadsafe() and waits for the result, so sync_execute, sync_get_response and the sync socket and serial code can go. With the loop in its own thread, the sync client also keeps working when the caller already runs a loop, e.g. in Jupyter.

Chunks, each with its tests and keeping the documented sync API:

1. ModbusTcpClient
2. ModbusTlsClient and ModbusUdpClient
3. ModbusSerialClient
4. remove the sync code in the transaction manager and transport that is then unused

Is this what you had in mind, or would you rather run a private loop with run_until_complete() on each call and avoid the thread?

[janiversen]

Your plan sounds logical, I only have one unknown, does asyncio.run_coroutine_threadsafe() impact the performance negatively ? Would it be a good idea to make a small example, with a function that eg. have a loop to a million. call it directly (sync) and via asyncio...while measuring the time

I have been toying with a third idea. Start a new loop in the client init, and then use futures to send/receive the execute call. But admitted I never found time to actually try it out.

Making the changes as you suggest, means the client changes can be isolated in mixin.py (the execute method) and thus in one go, be available for all clients....of course there will be some cleanup in each client.

Can I politely suggest, that you make a PR very early, because then it is a lot easier to discuss the actual code.

Thanks for taking this challenge, it is currently the biggest maintenance bottleneck, having 2 implementations the constantly differ slightly.

[sankalpsthakur]

I measured it on a Mac with Python 3.12: a local server, 5000 reads of 10 registers, median of 5 runs.

client	per request
sync client today	76 us
async client, awaited directly	101 us
async client on a loop thread, run_coroutine_threadsafe()	150 us
async client on a private loop, run_until_complete() per call	134 us

For a function that does nothing, the call itself costs 39 us through the thread and 29 us with run_until_complete().

So the thread adds about 75 us per request against the sync client today. Over a network or a serial line that disappears: reading 10 registers at 9600 baud alone takes about 34 ms on the wire (33 bytes at 8N1).

Your idea, a loop created in __init__ and run per call, is the run_until_complete() row. It is a bit faster, but it raises RuntimeError: Cannot run the event loop while another loop is running when the caller already runs a loop (Jupyter, or a sync client used inside an async app). The thread avoids that, so I'd keep it.

I'll open a draft PR with the tcp client so we can discuss the code.

[janiversen]

Lets continue with the threadsafe solution, I expected a much bigger difference, thanks for investigating.

I will continue in the PR.