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

Handle multiple threads in rails system tests #2287

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

Conversation

throwern
Copy link

I've been working on upgrading rspec tests in a Rails 6.0.4 application using activerecord-oracle_enhanced-adapter v6.0.6. This update demonstrates a bug we encountered and a potential fix. The bug RuntimeError: executing in another thread occurs for us after the following:

  • configuring rspec to use rails System tests
  • setting config.use_transactional_fixtures = true
  • running a system spec configured with js: true that contains ajax requests and many interactions with the database

The error seems to be a race condition and is difficult to reproduce consistently. After tracing through the code, I think it is related to a feature added in Rails 5.1 to support transactions in system tests: rails/rails#28083

The update made all threads share the same database connection in tests. It uses Monitor and synchronize to avoid simultaneous requests from different threads. For example, anything using the log method is wrapped with a call to synchronize: https://github.com/rails/rails/blob/v7.0.2.3/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L765

It seems that some methods in the oracle adapter are not synchronized. For our project, the errors traced back to the OracleEnhanced::Connection#describe method in connection.rb. I was able to fix the error with calls to @lock.synchronize around any method using describe. Those changes are included in this commit.

I'm not deeply familiar with this adapter or ruby threads so this is more of a quick patch and proof-of-concept. I included a test case that usually reproduces the error without the fix. Hopefully someone more familiar with these codebases could take a look.

Thanks!

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.

1 participant