socketry / falcon

A high-performance web server for Ruby, supporting HTTP/1, HTTP/2 and TLS.
https://socketry.github.io/falcon/
MIT License
2.54k stars 79 forks source link

Add more docs re: Rails #220

Closed trevorturk closed 5 months ago

trevorturk commented 5 months ago

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

Contribution

trevorturk commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Perhaps we can close https://github.com/socketry/falcon-rails-example/issues/2 once we get this documented?

Also, somewhat related to this and https://github.com/rails/rails/pull/50917, see also https://github.com/socketry/falcon-rails-example/issues/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!

trevorturk commented 5 months ago

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 commented 5 months ago

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

trevorturk commented 5 months ago

Updated here! https://github.com/socketry/falcon/pull/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!