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

TMF CoAP messages checking for Confirmable/Non-confirmable is too strict #10223

Closed
EskoDijk opened this issue May 13, 2024 · 4 comments
Closed

Comments

@EskoDijk
Copy link
Contributor

Describe the bug A clear and concise description of what the bug is.

In many places in the Openthread code, checks like below are performed on a received TMF message:

VerifyOrExit(aMessage.IsConfirmablePostRequest());

But in CoAP, the messaging layer (CON/NON) is separated from the request/response processing layer, and therefore the CON or NON transport does not impact the semantics of the request or response. Forcing a particular transport (CON/NON) at the receiver side seems to serve no purpose and also limits the full use of CoAP features in the future.

Please feel free to provide some comments here -- I won't make a PR for this if the basic assumptions are not agreed on, of course.

To Reproduce Information to reproduce the behavior, including:
N/A

Expected behavior A clear and concise description of what you expected to happen.
Only check for required CoAP message elements at the req/resp layer, such as request method (e.g. verify it is POST).

VerifyOrExit(aMessage.IsPostRequest());

Additional context
See RFC 7252

@abtink
Copy link
Member

abtink commented May 14, 2024

@EskoDijk, I agree, makes sense to me to relax the condition if it's permissible for specific commands.

@jwhui
Copy link
Member

jwhui commented May 15, 2024

Relaxing the constraint makes sense to me.

@EskoDijk
Copy link
Contributor Author

I checked the code for feasibility for this, and it turns out that due to the current design philosophy and current definition of transaction patterns in the Thread spec, this change is not easy to do.

Examples:

  • handlers of a CON POST will unconditionally send an empty Ack, if there's no response to be sent back. It doesn't depend on CON/NON of the request.
  • handlers that do send a response with content back, will always use the 'Ack' Type and this makes it hard to introduce the 'Non-confirmable' type which would be required in response to a NON request.

So allowing both NON/CON type requests could lead to new issues such as a device Ack'ing a NON.

All of this could be optimized by cleanly separating the CoAP reliability layer from handling the request/responses, but this is not the present design. It would in fact save quite a bit of code, because code for 'Acking' is now custom in every handling and this could be just a single CON-layer handler doing this.

Note that the present design also gives rise to behaviors that are not in line with the CoAP semantics: for example, if a CON request application-level handling fails somewhere, the ACK may not be sent, leading to repeated processing of the same CON request due to retransmissions! Formally, the recipient needs to ACK the request even if the payload/app-layer processing of it failed - it was received, after all. The sender ought not to resend the same request again per CoAP semantics.

Anyhow, this is a bit too much of a change - instead I have a PR with some bugs caught in the doc/naming: see #10615

@EskoDijk
Copy link
Contributor Author

Closing this issue, since it's not easily solvable with current CoAP TMF specified behavior and current sw design; and there's no real issue keeping the checks strict.

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