identity: refactor cache directory to prevent nil/nil condition #479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bnewbold
changed the title
whyrusleeping
added a commit
that referenced
this pull request
Oct 7, 2024
A bunch of cleanups and bug fixes. The biggest fix is to apply a bug fix from the cache directory resulting in "nil/nil" responses. These were fixed in #479, but not copied to the redis directory implementation, which has the same coalescing code. This recently impacted @whyrusleeping's use in discover. This is a backwards-compatible fix. Another fix handles cached error messages. An empty string was being stored for the DID. Because redis cache serializes, and the de-serialized DID is re-parsed, the empty strings fail to parse, resulting in an error. I guess we could consider having text parsing of DIDs not actually parse the string? but that would break auto-validation when it is expected. The fix was to make that field nullable. this might make the change not-backwards-compatible with existing cached metadata? this only impacts cached errors, which have a shorter TTL, so might not be that big of a deal in practice? Other smaller cleanups and fixes, which rolled in here to make the code align better with the in-memory caching directory, so that merge/review is easier: - adds invalid handle TTL to redisdir - adds "WithCacheState" variants of methods, for metrics/perf measurement - fix deletion/purging - special-case handling of resolving invalid handle One remaining issue is that the cached error types come back as generic errors (with error kind/type striped). It would be nice if that wasn't the case, so that code could detect specific types of errors? Considering doing a manual enum/integer hack to encode the type and convert the errors to the actual type on read, for a small set of known identity errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is closely related to #458, which tried to resolve an issue with DIDs. I was seeing very confusing behavior with handles; was able to confirm
"", nilgetting returned internally on real-world lookups.I took a more aggressive approach and changed the return signatures of both update helper methods to just return an "Entry", not as a pointer, and no err. This makes the code flow much simpler.
I tacked on a special control-flow error message as defensive programming after getting bit twice on this (darn self!). Probably isn't necessary and smells a bit but 🤷.