Bug 1275 - cached data in cachedb is never used
cached data in cachedb is never used
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-02 00:44 CEST by JINMEI Tatuya
Modified: 2017-06-08 00:53 CEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2017-06-02 00:44:44 CEST
As of trunk revision 4204, data cached in the cachedb module doesn't seem
to be ever used.  One of the validations of the cached data,
cachedb.c:parse_data() always returns 0, so the validation never succeeds.

I confirmed it by the following simple test:
- start unbound with
  module-config: "validator cachedb iterator"
- send a query, confirm the response is stored in cachedb:
  [1496356416] unbound[11273:0] debug: testframe_lookup of E4AD6C08E45076354BEA1208464F75D3DF52BB9364ABE038E72422667048430D
- flush the "internal" cache: unbound-control flush a.example.com
- send the same query, confirm unbound finds the stored cache
  [1496356426] unbound[11273:0] debug: testframe_lookup of E4AD6C08E45076354BEA1208464F75D3DF52BB9364ABE038E72422667048430D
  [1496356426] unbound[11273:0] debug: testframe_lookup found 98 bytes
- also confirm it still performs iterative resolution

An obvious "fix" is to change the final return of parse_data() so
it will return '1' (I confirmed it worked as I expected), but perhaps
the current behavior is intentional (maybe because the "TODO" isn't
implemented yet)?  If so, maybe it's better to leave a comment about it
so other readers won't be confused.

Index: cachedb/cachedb.c
===================================================================
--- cachedb/cachedb.c	(revision 4204)
+++ cachedb/cachedb.c	(working copy)
@@ -438,7 +438,7 @@
 		*/
 	/* TODO */
 
-	return 0;
+	return 1;
 }
 
 /**
Comment 1 Wouter Wijngaards 2017-06-06 14:10:07 CEST
Hi Jinmei,

Yes, I implemented the TODO, and committed it.  This decrements the TTL of the picked up message by the difference of (storage_time - current_time).  (Should this have an option to pick up an expired message if serve-expired is on?  How will that then refetch expired messages?).

Best regards, Wouter
Comment 2 JINMEI Tatuya 2017-06-08 00:53:25 CEST
Thanks for the fix.

As for the interaction with serve-expired, yes, I think we need some way to use otherwise-expired cachedb data when we also enable serve-expired.  For refetch we'll need to distinguish a resolution attempt for refetch from an attempt due to cache miss so the former will bypass cachedb.  Conceptually it shouldn't be that hard (I guess you're aware of that) but it may require tricky implementation in practice.