Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client sending no_response option 26 keep active tasks #170

Open
HRogge opened this issue Oct 1, 2019 · 18 comments
Open

Client sending no_response option 26 keep active tasks #170

HRogge opened this issue Oct 1, 2019 · 18 comments
Assignees

Comments

@HRogge
Copy link
Contributor

HRogge commented Oct 1, 2019

I am experimenting with both unicast and multicast and have trouble with the no_response option.
I have set the option (both on client transmission taks and on server resource) to 26 to make sure no response is generated (important for multicast), but the client (modified clientGET.py) keeps two tasks running anyways, most likely waiting for a response.

my clientPUT.py code:

    context = await Context.create_client_context()

    await asyncio.sleep(2)

    payload = b"The quick brown fox jumps over the lazy dog.\n"
    request = Message(code=PUT, mtype=NON, no_response=26, payload=payload, uri="coap://[fe80::250:56ff:fea8:e21%eth2]/other/separate")

    response = await context.request(request).response

    print('Result: %s\n%r'%(response.code, response.payload))

output of clientPUT.py after terminated with CTRL-C:

KeyboardInterrupt
ERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending coro=<BlockwiseRequest._run_outer() running at /root/aiocoap/aiocoap/protocol.py:722> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fcb3c7e4a68>()]>>
ERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending coro=<Request._run() running at /root/aiocoap/aiocoap/protocol.py:608> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7fcb3c7e4be8>()]>>

Is there something special when sending a Message which should get no response?

@chrysn
Copy link
Owner

chrysn commented Oct 1, 2019

To be honest, support for NoResponse was primarily put in on the server side – I didn't think of the client side yet.

The intended behavior for NoResponse:26 would indeed be to not have a task active, other codes would need to time out.

Thanks for reporting. Unfortunately I don't see any short-time wkrarounds (what did I think discarding the task reference when creating it…), I'll try to come up with something.

@chrysn chrysn self-assigned this Oct 1, 2019
@HRogge
Copy link
Contributor Author

HRogge commented Oct 1, 2019

Some kind of timeout would be fine as a workaround for 26 too... keep the tasks active for some seconds, then stop them. Would it be possible to add an additional timer (when no_response != 0) that cancels the running task(s)?

There might be also a problem when combining "no_response" with a payload large enough for fragmentation to activate... I am still trying to understand the sections in protocol.py.

@chrysn
Copy link
Owner

chrysn commented Oct 1, 2019

aiocoap does not implement a timeout by itself (leaving that to the application), but should allow for the task to be reaped once the application lost interest in the request. So in the end, once you drop your reference to the request (or explicitly cancel it), the active request task should stop itself (cease to way / stop retransmitting / stop fetching new blocks) – which is what I'm effectively treating this issue as.

NoResponse and block-wise can be combined in theory, but I only see that applicable in larger schemes (eg. when someone specifies an error payload for "Can't accept this block because the earlier blocks 23, 42 and 47 are missing").

@HRogge
Copy link
Contributor Author

HRogge commented Oct 1, 2019

Okay, I tried the following code

    future_response = context.request(request).response
    try:
        response = await asyncio.wait_for(future_response, timeout=2) 
        print('Result: %s\n%r'%(response.code, response.payload))
    except asyncio.TimeoutError: 
        return

but I still get one error that a task is not properly canceled:

DEBUG:coap:Sending request - Token: dcb9, Remote: <UDP6EndpointAddress [('fe80::250:56ff:fea8:e21', 5683, 0, 10)]>
DEBUG:coap:Sending message <aiocoap.Message at 0x7f7172eb1c88: Type.NON PUT (MID 54508, token dcb9) remote <UDP6EndpointAddress [('fe80::250:56ff:fea8:e21', 5683, 0, 10)]>, 2 option(s), 45 byte(s) payload> (None)
ERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending coro=<Request._run() done, defined at /root/aiocoap/aiocoap/protocol.py:605> wait_for=<Future cancelled>>

@chrysn
Copy link
Owner

chrysn commented Oct 1, 2019

Yes, that's because those tasks are not reaped yet – working on it :-)

(But that's about the invocation I'd expect to work in the end, although introducing a shortcut may make sense).

@HRogge
Copy link
Contributor Author

HRogge commented Oct 1, 2019

I got it working with the following change:

diff --git a/aiocoap/protocol.py b/aiocoap/protocol.py
index 7547497..8c739b4 100644
--- a/aiocoap/protocol.py
+++ b/aiocoap/protocol.py
@@ -594,10 +594,16 @@ class Request(interfaces.Request, BaseUnicastRequest):
         else:
             self.observation = None
 
-        loop.create_task(self._run())
+        self._runner = loop.create_task(self._run())
 
         self.log = log
 
+        self.response.add_done_callback(self._response_cancellation_handler)
+
+    def _response_cancellation_handler(self, response):
+        if self.response.cancelled() and not self._runner.cancelled():
+            self._runner.cancel()
+
     @staticmethod
     def _add_response_properties(response, request):
         response.request = request

just a copy of the cancellation handling of the blockwise request.

(edit: also works for multicast transmission)

@chrysn
Copy link
Owner

chrysn commented Oct 1, 2019

What bugs me a bit about the "request and cancel" approach is that the request operation, even though it is not expecting a response, may not be complete yet. Having a CON in there is (in most cases) missing the point of NoResponse, but one of the additions I've been thinking about is implementing the burst mode suggested for NONs (transmitting multiple copies, see RFC7252 Section 4.3), and that you may want to continue.

In the end there'll need to be a distinction between "I'm not interested in the response" and "I'm not interested in getting this sent any more", and I don't have a clean concept for them yet.

@HRogge
Copy link
Contributor Author

HRogge commented Oct 2, 2019

Please make this "burst mode" optional or at least configurable... depending on the "upper layer" protocol the application might just not care if some transmissions are lost, especially in the case of multicast.

This is especially true for Hello/Discovery style protocols, synchronization of fast changing data or routing protocols.

Could you maybe add a callback when the transmission was done? In this case this callback could be used to cancel the request by the application.

@HRogge
Copy link
Contributor Author

HRogge commented Mar 24, 2020

Any news on this? I am starting my work on COAP again and I really need the no-response mechanism. Using COAP is much better than building your own protocol over UDP.

I updated my own fork of your code and noticed the changes above have not been merged.

@chrysn
Copy link
Owner

chrysn commented Mar 24, 2020

I lost sight of this in October. I've merged your patch (so switching over to the master branch should work for your application) but still look into how I can test it (ie. ensure that the the cancellation passed on even in combination with future code changes).

The issue will stay open for the remaining question, which I'd summarize as "but when may the application cancel the request to be sure it at least got submitted?" (especially in the light of different transports behaving differently, think TCP with Nagle, or transports that need an initial handshake where a 0RT message may or may not be accepted). My gut feeling here is that in order to ensure network friendliness, the application can't rely on a particular message to be transmitted anyway (observations have this in as "eventual consistency", and any form of POST would behave the same). Then, an application that sends a continuous stream of No-Response messages should always keep one request around, and cancel it by the time it wants to send the next one. Would that work for your application?

chrysn added a commit that referenced this issue Mar 24, 2020
The previous commit did cancel the runner, but that did not effect the
underlying plumbing request.

A test has been added to verify that the plumbing request has vanished
from the token manager's view.

See-Also: #170
@chrysn
Copy link
Owner

chrysn commented Mar 24, 2020

While adding a test I found that some of the state was still lingering with your patch; 9b249a2 fixes that, and should now leave you with no lingering state if the response is always cancelled in time.

@HRogge
Copy link
Contributor Author

HRogge commented Mar 24, 2020

That sounds great... I will continue to work on my aiocoap code and come back if I verified that the current master works for me.

@HRogge
Copy link
Contributor Author

HRogge commented Mar 24, 2020

I just finished some testing and have still trouble with the client sending a non-response message.

This is the debug output of the client:
DEBUG:coap:Sending request - Token: 8dde, Remote: <UDP6EndpointAddress [::ffff:127.0.0.1]:5683>
DEBUG:coap:Sending message <aiocoap.Message at 0x7faccee5fa20: Type.NON POST (MID 5019, token 8dde) remote <UDP6EndpointAddress [::ffff:127.0.0.1]:5683>, 3 option(s), 31 byte(s) payload>
^CERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending coro=<Request._run() done, defined at /home/henning.rogge/.local/lib/python3.7/site-packages/aiocoap/protocol.py:605> wait_for=>
ERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending coro=<BlockwiseRequest._run_outer() running at /home/henning.rogge/.local/lib/python3.7/site-packages/aiocoap/protocol.py:720> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7faccee69648>()]>>

@HRogge
Copy link
Contributor Author

HRogge commented Mar 25, 2020

I just updated to commit 9b249a2 (from yesterday), but the problem is still there.

From what I see the function _response_cancellation_handler() (protocol.py, line 603) is never called when my program ends.

@chrysn
Copy link
Owner

chrysn commented Mar 25, 2020 via email

@HRogge
Copy link
Contributor Author

HRogge commented Mar 25, 2020

At the moment my test-code in the client sends a single message and then the client program ends... no second transmission at all.

request = aiocoap.Message(code=aiocoap.POST, mtype=aiocoap.NON, no_response=26, payload=cbor_payload, uri=uri)

response = await self.protocol.request(request).response

I would expect that I get a dummy response to tell me "yes, transmission has happened, move along"...

Maybe I am missing something?

@HRogge
Copy link
Contributor Author

HRogge commented Mar 25, 2020

I think I found a solution by adding a (fixed) timeout to the handling of plumbing events for no-response messages.

see #190

@marianhlavac
Copy link

Stumbled upon the same problem, aiocoap isn't handling outgoing NON messages correctly. Any luck with the issue, or opened PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants