Fix thread-safety issues in acts_as_xapian for multi-threaded servers#9022
Open
gfrmin wants to merge 6 commits intomysociety:developfrom
Open
Fix thread-safety issues in acts_as_xapian for multi-threaded servers#9022gfrmin wants to merge 6 commits intomysociety:developfrom
gfrmin wants to merge 6 commits intomysociety:developfrom
Conversation
The Xapian database connections, enquire objects, and query parsers were stored in class variables (@@db, @@Enquire, @@query_parser), which are shared across all threads in a multi-threaded web server like Puma. This causes race conditions where one thread's search operations can interfere with another thread's operations, leading to crashes and incorrect search results. This commit converts these to thread-local storage using Thread.current, ensuring each thread has its own independent Xapian connection. Fixes issues in Puma deployments with RAILS_MAX_THREADS > 1.
Member
|
@gfrmin Thanks for this and the other PRs. Could you look at the failures in the test examples and the Rubocop warnings. |
Member
|
Marking as draft since the specs are failing. |
Thread-local storage setters cannot use attr_accessor as they need to write to Thread.current hash keys. These are intentionally "trivial" wrappers around thread-local storage for thread-safety. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This completes the thread-safety fix started in commit 166f30f. That commit converted db, enquire, and query_parser to thread-local storage but missed the four metadata dictionaries that track per-query-parser state. This commit converts the remaining class variables to thread-local storage using the same pattern: - @@values_by_prefix → Thread.current[:acts_as_xapian_values_by_prefix] - @@values_by_number → Thread.current[:acts_as_xapian_values_by_number] - @@terms_by_capital → Thread.current[:acts_as_xapian_terms_by_capital] - @@value_ranges_store → Thread.current[:acts_as_xapian_value_ranges_store] Changes: - Added getter/setter methods for all 4 dictionaries (lines 113-145) - Updated initialization in init_query_parser to use setters (lines 259-262) - Replaced all @@ references with self. throughout the file This fixes the race condition where Thread A's query parser metadata could be wiped out when Thread B calls readable_init, causing "couldn't find prefix" errors in multi-threaded Puma deployments. Fixes: RuntimeError in request#similar action under concurrent load 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
gfrmin
force-pushed
the
fix-xapian-thread-safety
branch
from
d964a79 to
fc0136c
Compare
Fixes two issues in the thread-safety conversion: 1. writable_init was setting @@Enquire (class variable) instead of using the thread-local setter, causing enquire to return nil 2. Removes redundant self. prefix from query_parser calls (RuboCop Style/RedundantSelf) This resolves the "undefined method 'query=' for nil" error in xapian_index. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Addresses all RuboCop violations reported in PR CI: - Remove redundant rubocop:disable/enable directives for Style/TrivialAccessors - Remove redundant self. prefix from method calls where not required - Fix line length violations by splitting long conditional statements All changes are style-only with no functional changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
Tests pass now, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes critical thread-safety issues in the acts_as_xapian module that affect multi-threaded web servers like Puma.
Problem
The Xapian database connections, enquire objects, and query parsers were stored in class variables (
@@db,@@enquire,@@query_parser), which are shared across all threads in a multi-threaded web server. This causes race conditions where one thread's search operations interfere with another thread's operations, leading to:Solution
This commit converts these shared class variables to thread-local storage using
Thread.current, ensuring each thread has its own independent Xapian connection.Impact
RAILS_MAX_THREADS > 1(default is 3 threads)Testing
Tested in production environment with Puma running 3 threads per worker. Previously experiencing intermittent search failures, now stable.
Files Changed
lib/acts_as_xapian/acts_as_xapian.rb- Converted@@db,@@enquire, and@@query_parserto thread-local storage