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

fix(tokenmanager): only use new token if not set #262

Closed
wants to merge 1 commit into from

Conversation

mguc
Copy link

@mguc mguc commented Nov 15, 2021

When setting a message token manually in a message like

request = Message(code=GET, uri='coap://localhost/time', token=b'1234')

the value gets overridden in the aiocoap/tokenmanager.py. This PR makes sure that the provided value is used.

This useful if you want to send a message with the same token from a previous request. I need this functionality to send a de-registering (request.opt.observe = 1) request to a CoAP server.

@chrysn
Copy link
Owner

chrysn commented Nov 16, 2021

Thanks for contributing this as a PR. This fixes a symptom of the long-standing lack of cancellation (#92).

This patch should stay functional for until that is fixed, and it's good to have it as a workaround -- merging it into the main development branch appears to make little sense as it can't be used without further tools to send cancellations.

I'll take this as a hint to priorize cancellation :-)

@mguc
Copy link
Author

mguc commented Nov 17, 2021

I still think that it could be useful since the Message class provides this argument and currently it is ignored. There are probably other scenarios (not only for observe messages) where setting the token manually could be meaningful. But yeah, I leave it up to you :-)

@chrysn
Copy link
Owner

chrysn commented Nov 17, 2021

That the message class exposes token (and message ID) at all is a relic of before the introduction of the standalone token and message managers; it predates the existence of different transports like CoAP-over-TCP.

Let's explore some scenarios where this could be needed, maybe something is in there that indeed has merit:

  1. Non-observe multiple responses: Formalized in a proposed update to core-responses. Same as for observation applies: Setting the token here would terribly confuse the library. (I think the way to go here is to build the follow-up from the original request).
  2. Extended token lengths for stateless processing (RFC8974): Not implemented in aiocoap yet at all (should be straightforward though). With this it'd make sense for the application to set the token in a request. To actually be useful, aiocoap would need to forget about the request from then on, and let the application set a generic response handler for any incoming response (where then the application would decode the token).
    I'm not sure there are use cases for this in aiocoap (the RFC is aimed at constrained proxies that can't afford to store client addresses and thus place them in the token to be included in the response), but if there are, aiocoap needs extensions in other places.

I'll keep this open until we have a better understanding of the potential use cases, maybe a way forward with 2.

@chrysn
Copy link
Owner

chrysn commented Nov 29, 2023

Gathering implementation experience from the Rust side, I think that even if we add a requests-with-long-token-state interface, that'll not be through explicit token setting, but by starting something request-ish that picks a token prefix, and then allows application data in a suffix of that token.

In use cases, this was recently asked about (in #331), but that was just once more running into #92.

@chrysn chrysn closed this Nov 29, 2023
@mguc mguc deleted the fix/message_token branch December 8, 2023 10:06
@sikam
Copy link

sikam commented Sep 6, 2024

Hi @chrysn
In our project we also need to explicitly send a cancellation to the existing observations. Since this PR is not being merged, how would you suggest to cancel a subscription in a server (needs to be done with the same token as the subscription itself and OBSERVE flag set to 1)?

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

Successfully merging this pull request may close these issues.

3 participants