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

Extract thread cache key handling #106

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

getand
Copy link
Member

@getand getand commented Nov 9, 2021

Needed for thread cache key clearing for threaded application servers

@getand getand force-pushed the extract-thread-cache-key-handling branch from 430365b to 45b3c71 Compare November 10, 2021 08:27
@getand getand force-pushed the extract-thread-cache-key-handling branch from 45b3c71 to 6badb77 Compare November 10, 2021 08:30
@getand getand requested a review from a-berg-gs November 10, 2021 19:15
NOTE: In order to avoid problems with threaded application servers you should always clear the cache key one your
threads once the request is finished using `NxtHttpClient.clear_thread_cache_key` to not have other threads use the
same cache key and thus use cached results across threads.

Choose a reason for hiding this comment

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

So before a thread could handle a request and then another but keep its cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically if an application server reuses a thread the thread local variable need to be cleared so it's not bleeding into another request. Because the idea of caching per thread actually is to cache within a single request.

Co-authored-by: Andreas Berg <[email protected]>
Comment on lines +22 to +26
Thread.current[THREAD_CACHE_KEY] ||= build_thread_cache_key
end

def clear_thread_cache_key
Thread.current[THREAD_CACHE_KEY] = nil

Choose a reason for hiding this comment

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

What do you think about using RequestStore gem?

@rkallensee rkallensee changed the base branch from master to main February 7, 2022 12:51
@AkihikoITOH
Copy link
Contributor

Do we still need this, or can we close the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants