Feature/fix blockwise requires message by Theoyor · Pull Request #619 · plgd-dev/go-coap

Theoyor · 2025-09-11T11:40:32Z

Fixes #512

Summary by CodeRabbit

New Features
- Improved reliability of blockwise transfers with exchange-based keying, enhancing concurrency and handling when tokens are absent.
Refactor
- Updated blockwise APIs to require a remote address parameter; integrated across TCP/UDP clients. Adjust your calls to pass the connection’s RemoteAddr().
Chores
- Added IDE configuration and VCS mapping files.
- Expanded IDE/git ignore rules.
- Updated example client to use UDP port 5685 by default.

coderabbitai · 2025-09-11T11:40:40Z

Walkthrough

Introduces IDE configuration files under .idea, adjusts example client port, and overhauls blockwise logic to use an exchangeKey derived from remote address, path, and code. BlockWise Do/Handle signatures gain a net.Addr parameter, with TCP/UDP clients updated to pass RemoteAddr(). Token-based cache keys are replaced or complemented by exchangeKey.

Changes

Cohort / File(s)	Summary
IDE configuration `.idea/.gitignore`, `.idea/go-coap.iml`, `.idea/modules.xml`, `.idea/vcs.xml`	Added IDE metadata: ignore patterns, module file, modules registry, and VCS mapping. No runtime/code changes.
Example client port `examples/simple/client/main.go`	Changed UDP dial target port from 5688 to 5685.
Blockwise core `net/blockwise/blockwise.go`	Added exchangeKey computation (addr+path+code via FNV-1a). Public methods Do/Handle now accept `remoteAddr net.Addr`. Internal flows and caches switched to use exchangeKey (fallback to token when present). New helper `getExchangeKey`. Adjusted signatures across internal helpers.
Client conn wiring (TCP/UDP) `tcp/client/conn.go`, `udp/client/conn.go`	Propagated `RemoteAddr()` into blockwise.Do/Handle to match new signatures. No other logic changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as Client Code
  participant Conn as Conn (TCP/UDP)
  participant BW as BlockWise
  participant XKey as getExchangeKey
  participant Cache as BW Caches
  participant Net as Transport

  App->>Conn: Send CoAP request
  Conn->>BW: Do(req, szx, maxMsg, remoteAddr, inner)
  BW->>XKey: compute(addr, path, code)
  XKey-->>BW: exchangeKey
  BW->>Cache: lookup/store by exchangeKey
  BW->>Net: inner(req)
  Net-->>BW: response (blocks/complete)
  BW->>Cache: assemble/track by exchangeKey
  BW-->>Conn: final response

  Note over BW,XKey: If token missing, exchangeKey ensures routing per addr+path+code

  Conn->>BW: Handle(w, msg, szx, maxMsg, remoteAddr, next)
  BW->>XKey: compute if needed
  BW->>Cache: route receive/send via exchangeKey
  BW-->>Conn: next or continued transfer

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

jkralik

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title references "blockwise," which matches the primary area changed in the PR, so it is related to the changeset; however the phrasing ("Feature/fix" and "requires message") is ambiguous and does not clearly summarize the main work (exchange-based keying and addition of a remoteAddr parameter to Do/Handle).

Poem

A whisker twitch, a hop, a glide—
New keys exchange, aligned with stride.
From addr, path, and code we weave,
A tidy route for blocks to leave.
Ports now tuned, the clients hum—
Thump-thump! says bunny, here we come. 🐇✨

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title references "blockwise," which matches the primary area changed in the PR, so it is related to the changeset; however the phrasing ("Feature/fix" and "requires message") is ambiguous and does not clearly summarize the main work (exchange-based keying and addition of a remoteAddr parameter to Do/Handle).

Poem

A whisker twitch, a hop, a glide—
New keys exchange, aligned with stride.
From addr, path, and code we weave,
A tidy route for blocks to leave.
Ports now tuned, the clients hum—
Thump-thump! says bunny, here we come. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title Check	⚠️ Warning	The title “Feature/fix blockwise requires message” is vague and does not clearly convey the primary change, which is the introduction of exchange-based keying and remote address parameters to the blockwise Do and Handle methods. It also includes an unnecessary prefix and punctuation that do not add clarity. Overall, it fails to summarize the main feature update in a concise, readable way.	Please rewrite the title as a clear, single sentence reflecting the main change, for example: “Add exchange‐based keying and remote‐address support to blockwise Do and Handle” or similar.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tcp/client/conn.go (1)

210-214: Include Uri-Query in the fallback exchange key; Token is already used when present

BlockWise already uses r.Token().Hash() when Token != 0, so Token is handled.

getExchangeKey(addr, r) (net/blockwise/blockwise.go:getExchangeKey) currently hashes addr + Path only — it omits Uri‑Query (and request Code). When Token is empty this can cause concurrent exchanges to collide (same path, different queries).

Change the fallback to hash addr + full-URI (Path + Uri‑Query) + Code (or at minimum include Uri‑Query), and ensure identical key derivation is used everywhere that computes/uses exchangeKey (Do/handle/start/continue/getSentRequest paths in net/blockwise/blockwise.go).

♻️ Duplicate comments (3)

.idea/modules.xml (1)

1-8: Remove IDE module configuration

IDE-specific module configuration files should not be part of the repository.

.idea/vcs.xml (1)

1-6: Remove VCS configuration file

VCS mappings are IDE-specific and should be handled locally by each developer.

udp/client/conn.go (1)

443-449: Mirror the exchange-key uniqueness fix here as well.

Same concern as TCP: if exchangeKey ignores Token/Uri-Query, parallel requests to the same resource can interfere. Ensure blockwise.Do uses a key that includes Token when present.

See tcp/client/conn.go Lines 210-214.

🧹 Nitpick comments (4)

net/blockwise/blockwise.go (4)
887-888: Address the TODO comment about hash domain separation

The TODO comment indicates a potential hash collision concern that should be addressed to avoid systematic overlaps between token hashes and path/address hashes.

The TODO suggests separating hash domains to avoid collisions. Would you like me to implement a solution that uses different prefixes or hash spaces for token-based vs exchange-based keys?

585-588: Validate exchangeKey assignment logic

The conditional assignment of exchangeKey when it's 0 might mask issues where the exchange key should have been set earlier but wasn't.

Consider adding validation or logging to ensure the exchange key is being set as expected:
 if exchangeKey == 0 {
+    // This should only happen for observe responses or when token is used
     exchangeKey = sendingMessage.Token().Hash()
+    if exchangeKey == 0 {
+        return errors.New("invalid exchange key: both exchangeKey and token hash are zero")
+    }
 }
359-374: Improve error handling for exchange key generation

The path defaulting to "/" when empty might not be the intended behavior for all CoAP use cases. Also, the error message could be more informative.
 path, _ := r.Path()
 if len(path) == 0 {
-    path = "/"
-    print(path)
+    // RFC 7252 allows empty paths, but we need a default for the exchange key
+    path = "/"
 }

 var exchangeKey uint64
 if len(r.Token()) != 0 {
     exchangeKey = r.Token().Hash()
 } else {
     var err error
     exchangeKey, err = getExchangeKey(remoteAddr, r)
     if err != nil {
-        b.errors(fmt.Errorf("cannot get exchange key from request Parameters(%v): %w", r, err))
+        b.errors(fmt.Errorf("cannot generate exchange key for tokenless request (path: %s, addr: %v): %w", path, remoteAddr, err))
         return
     }
 }
161-179: Update function documentation

The comment on line 161 mentions that getSentRequestFromOutside must return a copy, but doesn't explain the new remoteAddr parameter's purpose in the blockwise context.

Add documentation explaining why remoteAddr is now required for blockwise transfers:
 // New provides blockwise.
-// getSentRequestFromOutside must return a copy of request which will be released after use.
+// remoteAddr is required to generate unique exchange keys for tokenless requests.
+// getSentRequestFromOutside must return a copy of request which will be released after use.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f431046 and fc584a0.

📒 Files selected for processing (8)

.idea/.gitignore (1 hunks)
.idea/go-coap.iml (1 hunks)
.idea/modules.xml (1 hunks)
.idea/vcs.xml (1 hunks)
examples/simple/client/main.go (1 hunks)
net/blockwise/blockwise.go (19 hunks)
tcp/client/conn.go (2 hunks)
udp/client/conn.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

examples/simple/client/main.go (1)

udp/client.go (1)

Dial (25-40)

udp/client/conn.go (2)

tcp/client/conn.go (1)

Conn (44-57)

mux/client.go (1)

Conn (17-51)

tcp/client/conn.go (2)

udp/client/conn.go (1)

Session (51-68)

tcp/client/session.go (1)

Session (22-43)

net/blockwise/blockwise.go (5)

message/pool/message.go (1)

Message (26-41)

message/message.go (1)

Message (12-21)

pkg/cache/cache.go (2)

NewElement (32-39)

Element (14-18)

net/responsewriter/responseWriter.go (2)

New (23-38)

ResponseWriter (17-21)

message/option.go (5)

Block1 (73-73)

Size1 (77-77)

Block2 (72-72)

Size2 (74-74)

OptionID (27-27)

🔇 Additional comments (3)

net/blockwise/blockwise.go (1)

208-226: Incorrect — Do already validates token and keys cache by token.Hash(). Do returns an error if len(r.Token()) == 0 and then LoadOrStore uses r.Token().Hash(); getExchangeKey(remoteAddr, r) is used in Handle when the token is empty.

Likely an incorrect or invalid review comment.

tcp/client/conn.go (1)

354-356: Good: remoteAddr threaded into BlockWise.Handle.

This keeps Do/Handle symmetry for keying. No issues here.

udp/client/conn.go (1)

666-673: Good: remoteAddr passed to BlockWise.Handle.

Matches the Do path; looks correct.

.idea/.gitignore

.idea/go-coap.iml

examples/simple/client/main.go

net/blockwise/blockwise.go

Theoyor · 2025-09-11T11:58:25Z

This PR is draft until:

I got feedback from an active contributor @jkralik wrote most of the blockwise code
I tested multiple scenarios including coap observe, handling of multiple blockwise transfers to different resources without tokens
I adjusted the unit tests to be correctly interacting with the proposed fix.

I would greatly appreciate feedback!

jkralik · 2025-09-13T08:45:12Z

This PR is draft until:

I got feedback from an active contributor @jkralik wrote most of the blockwise code

I tested multiple scenarios including coap observe, handling of multiple blockwise transfers to different resources without tokens

I adjusted the unit tests to be correctly interacting with the proposed fix.

I would greatly appreciate feedback!

Hi @Theoyor, thank you for your contribution. Here are my two cents on what I think needs to be done. Of course, this should also be covered by tests.

If the token is nil, only the remote address should be used as the exchange key. According to the CoAP blockwise transfer specification:

Token usage (RFC 7252 §5.3) with blockwise

The Token binds a request to its response.
If the Token is non-zero, multiple parallel exchanges with the same endpoint are possible, since each exchange can be disambiguated by (endpoint, token). However, per resource only one blockwise transfer may be active at a time.
If the Token is zero (nil), there is no disambiguation. In that case, only one transfer per endpoint can be active at a time. RFC 7252 §5.3

Summary:

Nil Token (0): at most one transfer per endpoint. In this case, if a nil token is used, the client must be limited to sending only one request–response exchange at a time. The client must wait until the previous exchange completes before sending a new request.
Non-nil Token: multiple transfers are possible, but only one per resource, disambiguated by Token.

Example with Nil Token

CLIENT                                                     SERVER
     |                                                              |
     | CON [MID=1234], POST /soap, Block1 NUM=0, M=1, Size=128 --> |
     |                                                              |
     | <------ ACK [MID=1234], 2.31 Continue, Block1 NUM=0, M=1    |
     |                                                              |
     | CON [MID=1235], POST /soap, Block1 NUM=1, M=1, Size=128 --> |
     |                                                              |
     | <------ ACK [MID=1235], 2.31 Continue, Block1 NUM=1, M=1    |
     |                                                              |
     | CON [MID=1236], POST /soap, Block1 NUM=2, M=0, Size=128 --> |
     |                                                              |
     | <------ ACK [MID=1236], Empty                                 |
     | <------ CON [MID=5000], 2.04 Changed, Block2 NUM=0, M=1      | <--- problem with disambiguation, only remote address could be used at CLIENT
     |                                                              |
     | ACK [MID=5000], POST /soap, Block2 NUM=0, M=1 -------------->|
     |                                                              |
     | <------ CON [MID=5001], 2.04 Changed, Block2 NUM=1, M=0      |
     |                                                              |
     | ACK [MID=5001], POST /soap, Block2 NUM=1, M=0 -------------->|
     |                                                              |
     ... (repeat for remaining Block2) ...

Theoyor · 2025-09-15T10:51:27Z

Hi @jkralik,

thanks for your review. Did you spot any issues with the code itself apart from the topic, you mentioned (and the tests)?

Regarding your comment, I copuldn't find the exact information you mantioned at RFC 7257 §5.3, however it says:

"The client SHOULD generate tokens in such a way that tokens currently in use for a given source/destination endpoint pair are unique. [...] An empty token value is appropriate e.g., when no other tokens are in use to a destination, [...]"

From which I infer that your statement is still valid and I will change the implementation accordingly.

…token.hash

…. unit-tests will be implemented later

jkralik · 2025-09-20T17:02:58Z

Hi @jkralik,

thanks for your review. Did you spot any issues with the code itself apart from the topic, you mentioned (and the tests)?

Regarding your comment, I copuldn't find the exact information you mantioned at RFC 7257 §5.3, however it says:

"The client SHOULD generate tokens in such a way that tokens currently in use for a given source/destination endpoint pair are unique. [...] An empty token value is appropriate e.g., when no other tokens are in use to a destination, [...]"

From which I infer that your statement is still valid and I will change the implementation accordingly.

Hi @Theoyor ,
Yes, my statement is inferred from 7252 §5.3.1.

An empty token value is appropriate e.g., when no other tokens are in use to a destination,  ...

Please address the fixes for the test, and also create a test with an empty token. From my side, the MR seems fine. Once these changes are in place, I will test them with our products, and then we can proceed with the merge.

jkralik · 2025-10-08T10:37:34Z

Hi @Theoyor,
I've created PR #626 from a branch in the coap repository. I'll work on fixing the test and CI issues there, so we can continue the discussion in that PR.

Theoyor marked this pull request as draft September 11, 2025 11:43

Theoyor mentioned this pull request Sep 11, 2025

Blockwise requests should be correlated based on message options #512

Closed

coderabbitai bot reviewed Sep 11, 2025

View reviewed changes

Theodor Rauch added 8 commits September 20, 2025 18:38

fix: switch to exchangeKey made from path and remote addr instead of …

6964900

…token.hash

fix: Only use alternative exchangeKey if no token is available

1f17223

revert blockwise_test to master as the changes were based on guessing…

74b9347

…. unit-tests will be implemented later

fix: remove .idea folder from git tracking

48c278f

fix: remove debug statement

a4ccff6

fix: remove comment

98d33e7

fix: more efficient hash generation

8a3f4a0

fix: only one request per endpoint (peer)

b4eb8a2

jkralik force-pushed the feature/fix-blockwise-requires-Message-ID branch from 35d9ce3 to b4eb8a2 Compare September 20, 2025 16:38

jkralik self-assigned this Oct 8, 2025

jkralik changed the base branch from master to jkralik/fix/blockwise-requires-Message-ID October 8, 2025 10:17

jkralik merged commit b4eb8a2 into plgd-dev:jkralik/fix/blockwise-requires-Message-ID Oct 8, 2025
7 of 12 checks passed

coderabbitai bot mentioned this pull request Oct 8, 2025

Feature/fix blockwise requires message #626

Open

coderabbitai bot mentioned this pull request Oct 15, 2025

Fix/tests blockwise requires message #627

Merged

jkralik linked an issue Feb 16, 2026 that may be closed by this pull request

Blockwise requests should be correlated based on message options #512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/fix blockwise requires message#619

Feature/fix blockwise requires message#619
jkralik merged 8 commits intoplgd-dev:jkralik/fix/blockwise-requires-Message-IDfrom
Theoyor:feature/fix-blockwise-requires-Message-ID

Theoyor commented Sep 11, 2025 •

edited by jkralik

Loading

Uh oh!

coderabbitai bot commented Sep 11, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Theoyor commented Sep 11, 2025

Uh oh!

jkralik commented Sep 13, 2025

Uh oh!

Theoyor commented Sep 15, 2025

Uh oh!

jkralik commented Sep 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

jkralik commented Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Theoyor commented Sep 11, 2025 • edited by jkralik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Pre-merge checks (2 passed, 1 warning)

Poem

Pre-merge checks (2 passed, 1 warning)

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Theoyor commented Sep 11, 2025

Uh oh!

jkralik commented Sep 13, 2025

Uh oh!

Theoyor commented Sep 15, 2025

Uh oh!

jkralik commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jkralik commented Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Theoyor commented Sep 11, 2025 •

edited by jkralik

Loading

coderabbitai bot commented Sep 11, 2025 •

edited

Loading

jkralik commented Sep 20, 2025 •

edited

Loading