#4337 Tweaks to make ActiveRecord Fiber-friendly - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4337 open

Tweaks to make ActiveRecord Fiber-friendly

Reported by Mike Perham | April 7th, 2010 @ 04:28 PM | in 3.1

My work on Phat (http://www.mikeperham.com/2010/04/03/introducing-phat-an-asynchrono...) brought to light a few issues in making Rails working in a single Thread, multiple Fiber environment.

The full set of patches required for ActiveRecord is here:

http://github.com/mperham/em_postgresql/blob/master/lib/active_reco...

Some of these changes can't be easily rolled back into Rails but the ones that can IMO are:

The connection pool's checkout method should mark a connection as checked out before actually using it, otherwise another Fiber may try to use the same connection while the verify! operation is in progress.

In connection_pool.rb

def checkout_and_verify(c)
  @checked_out << c
  c.verify!
  c.run_callbacks :checkout
  c
end

The connection pool Mutex should be configurable. Since Fibers cause a single thread to check out many connections from the pool, the Mutex causes deadlocks, even though there is only a single Thread executing. Ideally, I should be able to supply my own Mutex and Condition classes which are Fiber-friendly.

Comments and changes to this ticket

Jeremy Kemper April 7th, 2010 @ 06:24 PM
- Milestone cleared.
- State changed from “new” to “open”
- Assigned user set to “Jeremy Kemper”
Cool - thanks for working on this, Mike!

Looks like the pool implementation itself could be configurable so you could pick a threaded or fibered variant.

Patches (and tests) welcome, of course :)
You flagged this item as spam.
Mike Perham April 7th, 2010 @ 06:56 PM
Any suggestions on the switching logic? I like the idea of config.fibersafe! which works in a similar fashion to config.threadsafe! It would be able to rejigger the Rails stack (AR, memcache-client, e.g.) to use Fiber-safe versions.
You flagged this item as spam.
Mike Perham April 7th, 2010 @ 10:32 PM
- Tag set to “patch”
Patch attached. I'm unhappy with a few areas that perhaps you can suggest alternatives:
1. How do we cleanly get the concurrency_model value from the Rails::Application instance down to the ConnectionPool? My defined? code is definitely a hack.
2. I had to pull in EventMachine into the Gemfile in order to test the Fibers actually executing. The wait queue uses EM to pause while waiting for a connection and I'm not sure we want to introduce that dependency into ActiveRecord, even if it's pulled in dynamically.
3. There's no real way to implement remove_stale_cached_threads! since we can't get a list of Fibers in the system. I hacked it in em_postgresql by registering the application's FiberPool manually. I don't know if this is a showstopper but it's a concern.
- 4377-fibers-1.diff 10.2 KB
Dan Pickett May 15th, 2010 @ 01:54 AM
- Tag changed from “patch” to “bugmash, patch”
Advanced bugmashin': Can some bugmashers critique the patch and revise/comment as necessary?
Jeremy Kemper August 30th, 2010 @ 04:10 AM
- Milestone cleared.
- Importance changed from “” to “Medium”
[bulk edit]
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Ryan Bigg November 8th, 2010 @ 01:55 AM
- Tag cleared.
Automatic cleanup of spam.
Santiago Pastorino November 15th, 2010 @ 09:04 PM
- Milestone cleared.
[bulk edit]
You flagged this item as spam.
wtn November 20th, 2010 @ 05:59 AM
+1

Hope to see this patch get accepted.
José Valim November 20th, 2010 @ 10:08 AM
- Assigned user changed from “Jeremy Kemper” to “Aaron Patterson”
Aaron Patterson December 15th, 2010 @ 11:31 PM
- Milestone set to “3.1”
I will attempt to test / fix this. I don't like the architecture of the patch. I think we should have two subclasses of connection pool (one for threads and one for fibers) then switch the instantiation logic. Having initialize extend the current instance seems not good.

We can have the spec contain configuration info. IIRC, the spec comes from the database configuration yaml file, so config could be something like:
```
development:
  adapter: sqlite3
  database: db/development.sqlite3
  pool: 5
  timeout: 5000
  concurrency: fiber
```
(I'm not sure this is exactly what the config file would look like)

I'm marking this for Rails 3.1 as this seems like too much for a bugfix release.
You flagged this item as spam.
bingbing March 26th, 2011 @ 01:39 AM
omega watch