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

Blockwise requests should be correlated based on message options #512

Open
bergzand opened this issue Nov 27, 2023 · 4 comments · May be fixed by #554
Open

Blockwise requests should be correlated based on message options #512

bergzand opened this issue Nov 27, 2023 · 4 comments · May be fixed by #554
Labels
bug Something isn't working tech debt Technical debt

Comments

@bergzand
Copy link

Hello,

From what I understand from the current go-coap code, individual requests of the same block wise operation are identified based on an identical token of the individual requests. Unfortunately this is not correct per CoAP specification and go-coap only works with block wise with libraries that also have this peculiarity.

Instead of relying on the token, go-coap should rely on identical options between the individual requests, except for the NoCacheKey options and the block1/block2 options. See also the definition of "matchable" in rfc9175 that clarifies this.

Best regards,

@chrysn
Copy link

chrysn commented Nov 27, 2023

There has been a similar discussion around Zephyr. (The discussion is primarily of value for this issue if you want to take a look at how other implementations dealt with it, and for discussions around compatibility).

@bergzand, this issue is about the server side, right? (I.e., that a go-coap server needs the blocks to be carrying the same token). For the server side I don't think there are any special compatibility concerns (unless clients already abuse it to send concurrent block-wise transfers, which is way off spec). If go-coap can interop with itself, then the client side probably also sends the same token for each block of a multi-part request. That too is a behavior that may be worth reconsidering, especially if block requests are sent within EXCHANGE_LIFETIME, but if this issue is indeed about the server side, the client side may best be treated as a separate issue.

@Danielius1922
Copy link
Member

Danielius1922 commented Dec 1, 2023

@jkralik as far as I can see @bergzand and @chrysn are correct here and the current implementation is non-conforming to the RFC. Tokens can change within blocks of the same blockwise transfer.

@jkralik jkralik added bug Something isn't working tech debt Technical debt labels Dec 1, 2023
@roshangeoroy
Copy link

Is there any development on this issue? This issue makes go-coap incompatible with devices using OpenThread Library. OpenThread library does not keep any token at all for the consecutive GET requests.

@zworks-okada
Copy link

I needed the server to work for a quick prototype, and I ran into this issue using an older version of libCoAP as the client, so I tried implementing a fix.

I changed the receivingMessagesCache so it uses a hash generated from all the matchable options, code, and remote address instead of a hash generated from the token.

It's barely tested, but it did fix the issue which was occurring on my end where the server could only read the contents of the final block in the blockwise transfer (due to the token changing for every message).

master...zworks-okada:go-coap:bugfix/fix_serverside_blockwise_token_dependency

mpenate-ellenbytech added a commit to mpenate-ellenbytech/go-coap that referenced this issue May 15, 2024
Credit to @zworks-okada for initial work regarding rx transfers. Expanded to include tx.

Closes plgd-dev#512
mpenate-ellenbytech added a commit to mpenate-ellenbytech/go-coap that referenced this issue May 15, 2024
Credit to @zworks-okada for initial work regarding rx transfers. Expanded to include tx.

Closes plgd-dev#512
mpenate-ellenbytech added a commit to mpenate-ellenbytech/go-coap that referenced this issue May 15, 2024
Credit to @zworks-okada for initial work regarding rx transfers. Expanded to include tx.

Closes plgd-dev#512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tech debt Technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants