Skip to content

Add thread safety #20

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmewhort
Copy link
Contributor

These changes should makes rls_rails thread safe so that it can be used in multi-threaded applications -- such assidekiq, turbotests, etc.

Specifically, this includes the following changes:

  1. Use a thread-local variable for the thread status instead of a process-wide class instance variable.
  2. When a connection is checked out, reset the database session variables (and the thread local status variable to correspond). This ensures there's no leaking between threads, given ActiveRecord uses the same connection pool between different threads, with the different threads checking out new connections from the pool.

@sbiastoch
Copy link
Member

Hey @kmewhort, thanks a lot for this cool improvement! I just set up Github Actions - can you rebase and verify the tests are all passing?

Comment on lines +26 to +34
execute <<~SQL
RESET rls.user_id;
RESET rls.tenant_id;
RESET rls.disable;
SQL

clear_query_cache

RLS.thread_rls_status.merge!(tenant_id: '', user_id: '', disabled: '')
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to reuse here the existing RLS.reset! method (maybe with minor modifications) to not have this code duplicated here?

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.

2 participants