Re: [PATCH] OAuth 2.0 Bearer authentication

On 07/31/2014 03:29 AM, Amos Jeffries wrote:

> A garbage collection TTL "cleanup_interval" is configurable and removes
> cache entries which have been stale for at least 1 hr.

While some old code still uses periodic cleanup, I think we should avoid
adding more code like that. Periodic cleanup leads to cache overflows,
memory usage surprises, and hot idle. Please remove old unneeded cache
entries when adding a new cache entry instead of checking the cache
state every hour.

> + // only clear tokens out of cache after 1 hour stale
> + // could make this configurable
> + time_t stalenessLimit = current_time.tv_sec - 60*60;

Cache size should be(**) regulated in terms of memory usage (bytes), not
entry age (seconds). I believe estimating memory footprint of a cache
entry would be fairly straightforward in this case. Would you be willing
to convert the code to use size limits?

> + /*
> + * For big caches we could consider stepping through
> + * 100/200 entries at a time. Lets see how it flies
> + * first.
> + */
> + Auth::Bearer::TokenCache *cache = &Auth::Bearer::Token::Cache;
> + for (Auth::Bearer::TokenCache::iterator itr = cache->begin(); itr != cache->end();) {

I do not think we should do linear search like that. It is going to be
too slow for large caches and adding arbitrary "maximum number of
iterations" limits just adds more tuning headaches. Why not use an LRU
cache instead? IIRC, we have even added LruMap class for that recently.

> + For Basic and Bearer the default is "Squid proxy-caching web server".

Please add a comma after Bearer.

Thank you,

Alex.
(**) I hope we do not need to debate that rule of thumb, but I am ready
to justify it if needed.
Received on Mon Aug 04 2014 - 15:22:12 MDT