fix: jwt cache is not purged #3801
Conversation
| # the cache should now have three tokens | ||
| response = postgrest.admin.get("/metrics") | ||
| assert response.status_code == 200 | ||
| assert "pgrst_jwt_cache_size 3.0" in response.text |
There was a problem hiding this comment.
@taimoorzaeem Is it possible to have the size measured on bytes?
There was a problem hiding this comment.
Maybe. There is no built-in function for this in Data.Cache. I have tried using https://hackage.haskell.org/package/ghc-datasize to calculate size in bytes at runtime, but I have run into problems with that too.
We can try manually writing some logic to calculate the size in bytes, but I guess that would require some effort.
There was a problem hiding this comment.
There doesn't seem to be much code in that library: https://github.com/def-/ghc-datasize/blob/master/src/GHC/DataSize.hs. So maybe we can just vendor the code and use unsafePerformIO if it's cumbersome to integrate it.
There was a problem hiding this comment.
Btw, this metric could be a new PR so it's easier to review.
src/PostgREST/Auth.hs
Outdated
| checkCache <- C.lookup (getJwtCache appState) token | ||
|
|
||
| -- Call first to delete the expired JWTs | ||
| C.purgeExpired jwtCache |
There was a problem hiding this comment.
I am wondering if it is a good idea to add this in the hot path of request processing, unless we are sure it is a constant time operation (and fast).
Otherwise it should be moved to a separate thread.
There was a problem hiding this comment.
@mkleczek Good catch. The Data.Cache says:
purgeExpired :: (Eq k, Hashable v) => Cache k v -> IO ()Delete all items that are expired. This is one big atomic operation.
This indeed means that it is not a constant time operation so we should definitely move this to a separate thread.
Fixed the JWT cache not purging expired tokens in cache. This is tested by adding a new metric pgrst_jwt_cache_size.
taimoorzaeem
force-pushed
the
issue3788/jwt-purge
branch
from
5444935 to
7ffca04
Compare
| checkCache <- C.lookup (getJwtCache appState) token | ||
|
|
||
| -- Purge expired tokens in a separate thread | ||
| _ <- forkIO $ C.purgeExpired jwtCache |
There was a problem hiding this comment.
Hm, this is going to start a new thread on every request? That doesn't look efficient.
I think we should have a background thread that is notified for doing the purge.
There was a problem hiding this comment.
To be honest - for starters I would implement a simple solution that executes purgeExpired periodically ( frequency subject to configuration possibly - but not necessarily ).
Triggering purge upon every request might mean the purging thread is running constantly wasting cycles as there is nothing to do most of the times.
The ultimate solution would be to have some kind of scheduler that triggers purging upon next nearest expiry - but that would require more complex bookkeeping.
There was a problem hiding this comment.
Thinking about it some more - it seems to me the best moment to trigger purge is upon a cache miss - just before/after inserting a new entry.
The reasoning is that:
- We expect it to be rare (otherwise there is no point of the cache)
- It makes sure the cache is not growing (as inserting new entries does garbage collection)
- Since this is time expiration based cache there is no real risk of starvation - sooner or later we are going to have a cache miss.
There was a problem hiding this comment.
This seems like a much better idea. This way, we wouldn't need to do maintain any new thread state, hence avoiding extra overhead. @steve-chavez WDYT?
There was a problem hiding this comment.
Agree, looks like great idea 👍
There was a problem hiding this comment.
@taimoorzaeem not sure how to help with that - do you think it might be ready soon? Seems to me without it JWT caching is kind of useless (ie. has memory leak). I think it is a pretty good candidate for quick patch release.
There was a problem hiding this comment.
@mkleczek Yes, I agree that JWT caching is not production ready yet. @steve-chavez WDYT?
There was a problem hiding this comment.
Yes, we should definitely do a patch release. We need to merge #3802 first for testing, I'll review it tomorrow.
Fixes #3788.
Fixed the JWT cache not purging expired tokens in cache. This is tested by adding a new metric
pgrst_jwt_cache_size.@steve-chavez I can also divide this into two commits, one for adding the new metric and the other for bug fix. Please review and let me know.
Edit: I'll continue development once #3802 gets merged.