-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
[udp] make response message cache configurable #590
Conversation
WalkthroughThe changes introduce a new caching mechanism for CoAP messages in the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
c447896
to
b1f7350
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b1f7350
to
b80b7a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkralik any thoughts here? 🙂
Modifies UDP conn implementation to allow for a custom response message cache to be provided. This change is implemented in such a way that API compatibility with previous v3 releases is preserved. Users may now provide an option to supply their own cache implementation. Structured messages, rather than their serialized representation, are now passed to the cache implementation to allow for caching decisions to be made in the cache implementation. For example, it may be desirable to skip caching blockwise message responses if the entire underlying data being transferred is also cached. The cache implementation is responsible for cloning messages or otherwise ensuring that it is not storing data that may subsequently be modified. Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
b80b7a8
to
ac6c402
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
udp/client/conn.go
(8 hunks)
🔇 Additional comments (3)
udp/client/conn.go (3)
131-141
: LGTM! Well-designed cache interface.
The MessageCache
interface is well-defined with clear method signatures, allowing for custom cache implementations while maintaining a clean separation of concerns.
277-292
: LGTM! Well-documented and backward-compatible options.
The cache configuration options are well-implemented with:
- Clear documentation explaining the caching behavior
- Proper default initialization
- Backward compatibility maintained
Also applies to: 322-325
687-694
: Design note: MessageID-only cache key is intentional.
The implementation uses only MessageID as cache key, without including the remote address. This is intentional to support DTLS 1.2 Connection IDs where the underlying remote IP/port can change while the connection continues. In non-DTLS scenarios, Conn instances are not meant to be shared across endpoints.
Let's verify the DTLS connection ID usage:
✅ Verification successful
The search results confirm the presence of DTLS Connection ID support in the codebase, particularly in the examples/dtls/cid directory. Let's verify the specific implementation details:
Based on the evidence from the codebase, particularly the DTLS CID (Connection ID) example implementation, I can now generate the final response:
Design verification: MessageID-only cache key is correct and necessary
The implementation using only MessageID as cache key is verified to be correct. The codebase demonstrates DTLS Connection ID support where a connection can continue even when the underlying UDP endpoint changes. This is exemplified in the examples/dtls/cid/ implementation where:
- Server uses
RandomCIDGenerator(8)
to support connection migration - Client can resume connections from different UDP addresses while maintaining session state
- The cache design supports this by using MessageID-only keys, allowing message retrieval regardless of endpoint changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for DTLS connection ID related code
# Expect: Code showing DTLS connection ID handling
# Search for DTLS connection ID related code
rg -l "dtls.*connection.*id" --type go
Length of output: 40
Script:
#!/bin/bash
# Let's try different search patterns to find DTLS and connection ID related code
# Search for DTLS related code
rg "DTLS" --type go -A 5
# Search for ConnectionID related code
rg "ConnectionID" --type go -A 5
# Search for connection ID configuration or settings
rg "connection.*id" --type go -i -A 5
Length of output: 29093
Script:
#!/bin/bash
# Check the DTLS CID example implementation
cat examples/dtls/cid/server/main.go examples/dtls/cid/client/main.go
# Look for any cache-related code in the DTLS implementation
rg "cache" --type go -A 3 dtls/
Length of output: 5196
Thank you for your contribution ! |
Modifies UDP conn implementation to allow for a custom response message cache to be provided. This change is implemented in such a way that API compatibility with previous v3 releases is preserved. Users may now provide an option to supply their own cache implementation. Structured messages, rather than their serialized representation, are now passed to the cache implementation to allow for caching decisions to be made in the cache implementation. For example, it may be desirable to skip caching blockwise message responses if the entire underlying data being transferred is also cached. The cache implementation is responsible for cloning messages or otherwise ensuring that it is not storing data that may subsequently be modified.
Replaces #589
Summary by CodeRabbit
New Features
Improvements