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

Add more docs re: Rails #220

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

trevorturk
Copy link
Contributor

Add a bit more documentation around the isolation_level change from #219, including an example to retain the default behavior in Rails.

My concern is that with Rails <= 7.1.3 database connections are checked out until the response is sent back to the client. So, if you have a limited connection pool (as you might on Heroku etc) you might exhaust your limit unexpectedly if you have long-running fibers. In the future, Rails is likely to improve connection pool handling so this shouldn't be an issue in practice for long, but for now it seems prudent to document.

See #219 and #218 for more context, note links to other issues with other potential workarounds such as wrapping database queries in a with_connection block.

Types of Changes

  • Documentation

Contribution

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

We should also mention that per thread connections should not use transactions without also using with_connection or similar mechanism.

@trevorturk
Copy link
Contributor Author

trevorturk commented Feb 2, 2024

Gotcha. I updated with a bit more detail and added an example about the with_connection workaround.

Please let me know if you'd like me to squash these into a single commit or make any other tweaks. I'm not sure I'm following your comment about transactions specifically, so please elaborate if you wanted more detail added about that. My understanding is that you're advised to wrap any AR calls in a with_connection block if you have isolation level set to fiber, but perhaps there's more nuance there?

@ioquatix
Copy link
Member

ioquatix commented Feb 2, 2024

This is looking better and better. However, I think we should be explicit with the downside using :threads isolation causes w.r.t. transactions being shared across requests. Happy to merge this and revisit that in a separate PR, LMK what you prefer.

@trevorturk
Copy link
Contributor Author

Hmm, I'm not clear on the issue w.r.t transactions being shared across requests (sounds bad!) so perhaps you want to add to this, or merge and adjust/expand as you'd like?

@trevorturk
Copy link
Contributor Author

Perhaps we can close socketry/falcon-rails-example#2 once we get this documented?

Also, somewhat related to this and rails/rails#50917, see also socketry/falcon-rails-example#1 -- let me know if you'd like me to work on more documentation or a generator or something. I'd be happy to pitch in here if helpful!

@ioquatix ioquatix merged commit c9b36dd into socketry:main Feb 2, 2024
@trevorturk
Copy link
Contributor Author

Just to report back after a Rails 7.0.8 -> 7.1.3 upgrade today, I had some unexpected errors in production when running in :thread mode here: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb#L68 and then here: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L870 showing up as NoMethodErrors where results and then result were nil. I also had a smattering of ActiveModel::MissingAttributeError and ActiveRecord::SubclassNotFound errors that I hadn't seen before.

I checked with a Rails Slack group I'm in and heard that this has come up before, and that it seems to be related to multiple fibers sharing an Active Record connection. So, I implemented the with_connection block workaround and enabled :fiber isolation and things seem much better. I'm also happy to see that my db connection pool usage has barely increased. (I suppose checking the connection back in ASAP works just fine in practice.)

So, I wonder if we should remove the :thread workaround or perhaps add a caveat that leaving `:thread' in place seems to cause sporadic, difficult to understand/isolate issues like I experienced when using Rails 7.1.3.

@ioquatix
Copy link
Member

ioquatix commented Feb 2, 2024

That makes total sense to me - if you want to cut another PR to update the documentation that would be awesome!

@trevorturk
Copy link
Contributor Author

Updated here! #221

Please let me know how that looks and/or if you'd like any tweaks. Thanks again for your patience here, but I'm glad to share my experience in practice here and I hope it'll help others!

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