Skip to content

Conversation

snorrihann
Copy link

@snorrihann snorrihann commented Feb 16, 2024

According to https://datatracker.ietf.org/doc/html/rfc7252

In 4.1. Messages and Endpoints
An Empty message has the Code field set to 0.00. The Token Length
field MUST be set to 0 and bytes of data MUST NOT be present after
the Message ID field. If there are any bytes, they MUST be processed
as a message format error

In 5.2. Responses
After receiving and interpreting a request, a server responds with a
CoAP response that is matched to the request by means of a client-
generated token (Section 5.3); note that this is different from the
Message ID that matches a Confirmable message to its Acknowledgement.

@snorrihann snorrihann changed the title Ignore message ID check for response Use message token to match request and response, remove token from empty message Feb 16, 2024
@StFS
Copy link
Collaborator

StFS commented Feb 16, 2024

Okay, this is actually quite amazing!

I just ran the unit tests for CoAPthon3 in the following configurations:

  • The master branch of the original project
  • The master branch of the controlant-org project
  • The master branch of the snorrihann project (which this PR is based off)

Now get this... the unit tests hang in the first two projects on a test called test_separate!?! On the bugfix branch, that test passes!

So it seems that the unit tests that already existed were actually catching this bug.

There is a unit test that fails though (test_not_found) but that is failing on all 3 branches so that's probably not our fault.

I took a look at our GitLab pipelines that have been running these unit tests since we forked the project (https://gitlab.controlant.com/device-platform/libraries/third-party-libraries/CoAPthon3/-/pipelines) and can verify that all runs there have timed out on test_separate. I found only one test run in GitLab that had attempted the test_not_found test and that failed (https://gitlab.controlant.com/device-platform/libraries/third-party-libraries/CoAPthon3/-/jobs/1865322) so I think that's completely unrelated to this change.

@dachucky dachucky self-assigned this Feb 19, 2024
@dachucky dachucky self-requested a review February 19, 2024 12:04
@dachucky dachucky merged commit 6c607e7 into controlant-org:master Feb 19, 2024
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