-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gcoap: add nanocoap_cache support for clients #17888
Conversation
The "blockwise" noise is due to someone having been lazy in optimization on the aiocoap side. If the patch to aiocoap doesn't apply any more, it's likely because I'll pick most of it into aiocoap when I around to it; in that case, things will work with unmodified aiocoap already. (There won't be a Max-Age, but then again that only means testers will have to wait 60 rather than 10 seconds). |
22a5282
to
b646bab
Compare
Rebased |
Ok. The proxy definitely needs touching: as it stands, it caches the responses twice at the moment. Once for the server-side request (as the Proxy-URI option is part of the cache key) and once for the client-side request (as the for those the Proxy-URI option is stripped). With the client caching in place, the proxy only needs to do the validating part in place of the server, but none of the other cache handling parts. Will try to work that in. |
f13e814
to
584447d
Compare
Done |
9dbe1a9
to
921ce15
Compare
And rebased and squashed to current master, as #17881 was merged |
a1f7acb
to
74f4101
Compare
20c8d95
to
93c8585
Compare
And another rebase, now that #18091 was merged. Will do another round of testing now. |
Found some issues while testing. Those should now be fixed with the latest squash. |
To include the proxy, I also updated the testing procedures somewhat. However, the testing procedures in OP should also still work: Again I patched diff --git a/aiocoap/cli/fileserver.py b/aiocoap/cli/fileserver.py
index 1bf2ba4..f6ce129 100644
--- a/aiocoap/cli/fileserver.py
+++ b/aiocoap/cli/fileserver.py
@@ -160,2 +160,3 @@ class FileServer(Resource, aiocoap.interfaces.ObservableResource):
+ response.opt.max_age = 10
response.opt.etag = etag
Here is what Wireshark saw during the execution of commands First a large file |
8ac9385
to
e54f9ef
Compare
Changes look good so far. Squash Please. |
Most of the caching operation was moved to the client code. Since the forward proxy is using that code for upstream messaging, interacting with the cache directly is not necessary anymore. The only cache-related thing necessary for the proxy is validating ETags from upstream. However, that can be done by just looking at the ETags from the upstream response (which may or may not have come from the cache).
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.
Tested the caching functionality with and without a proxy. Works. Code also looks good. ACK
Contribution description
This provides caching support (via
nanocoap_cache
) forgcoap
clients. For the moment this is still a draft, as I am sure there is some cleanup possible now in caching parts of thegcoap_forward_proxy
module. However, I invite still everyone to review this PR.Testing procedure
Use
tapsetup
to create a TAP bridgePatch
aiocoap
to include theETag
andMax-Age
option in the responses of its file server examplePatch
Start the
aiocoap
file server example bound to the TAP bridge:Start a Wireshark instance and sniffing on the
tapbr0
interface.In another terminal, compile the
examples/gcoap
example including thenanocoap_cache
module and start it in a terminalSend a request for
/a.txt
, then directly after, another, then wait 10 seconds (or whatever value you gave theMax-Age
option when patching the file server) and send yet another request. All should show the same response:However, in Wireshark, you should only see two request/response pairs (the second request was answered from local cache), with the second pair having a 2.03 Valid (rather than a 2.05 Content) response that carries no payload (validating the already present, but stale, cache entry on the third request):
pr17888_test_procedure.pcapng
Issues/PRs references
Depends on
#17801(merged),#17881(merged), and#18095(merged) and all their dependencies.