Skip to content
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

IAM: Add caching to HTTP client #3148

Merged
merged 12 commits into from
Jun 4, 2024
Merged

IAM: Add caching to HTTP client #3148

merged 12 commits into from
Jun 4, 2024

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented May 27, 2024

Fixes #3142

@woutslakhorst @gerardsn is this an approach you agree on?

I think the Nuts node should having client-side caching features on board, since I've not often seen an outbound proxy, let alone one that performs caching. So I suspect most parties won't be considering an outbound proxy for caching capabilities. This could severely downgrade performance/UX, given how often the same resources are requested.

There are a few libraries that do this, but all of them were archived and unmaintained with reasons like:

  • This is so easy to implement, no need for a library
  • Caching is not needed in modern architectures (?)
  • Caching is very application-specific, so a library is too hard/complex/big

This solution just looks at expiry time (typically max-age), keeps cached resources around for max. 1 hour (or max-age if less). It also maxes out the available cache to 1mb (ofc. should be configurable).

@reinkrul reinkrul marked this pull request as ready for review May 27, 2024 19:11
@reinkrul reinkrul requested review from gerardsn and woutslakhorst and removed request for gerardsn May 27, 2024 19:11
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add an issue for adding metrics to the client cache for hits vs misses. (enterprise feature ;))

auth/client/iam/caching.go Outdated Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("error while reading response body for caching: %w", err)
}
h.mux.Lock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability purposes place a single lock/unlock at the beginning of the only public method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That causes IO to be included in the lock scope, so that causes scalability/performance issues. I refactored the function a bit to make it seem less random.

auth/client/iam/caching.go Outdated Show resolved Hide resolved
auth/client/iam/caching.go Outdated Show resolved Hide resolved
@gerardsn
Copy link
Member

out of interest / sanity check; how many simultaneous requests (order of magnitude) do you think it takes before the mutex is slower than ignoring cache? A successful authorization_code flow locks around 15-20 for the verifier and for the requester/holder

@reinkrul
Copy link
Member Author

out of interest / sanity check; how many simultaneous requests (order of magnitude) do you think it takes before the mutex is slower than ignoring cache? A successful authorization_code flow locks around 15-20 for the verifier and for the requester/holder

Locking is only done while checking whether there's a cached copy and when inserting an item into the cache (memory/CPU operations only), not during HTTP requests/IO. So caching with mutexes will be much, much faster than performing HTTP requests, even with many parallel connections (I'd say especially with many parallel connections).

@reinkrul reinkrul requested a review from woutslakhorst May 31, 2024 10:02
@reinkrul
Copy link
Member Author

reinkrul commented Jun 1, 2024

@woutslakhorst I noticed the did:web resolver calls weren't cached, since it creates its own HTTP client (through core.NewStrichtHTTPClient(). I moved the strict HTTP client to the http/client package so we can manage the caching HTTP client centrally.

v5 functionality does not use the caching HTTP client.

@reinkrul reinkrul merged commit 3734bcc into master Jun 4, 2024
9 checks passed
@reinkrul reinkrul deleted the iam/httpclient-caching branch June 4, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caching did:web documents, OAuth/OpenID metadata and StatusList2021
3 participants