ohler55 / agoo

A High Performance HTTP Server for Ruby
MIT License
912 stars 39 forks source link

fails to launch in clustered mode #108

Closed stakach closed 9 months ago

stakach commented 2 years ago

Trying to compare performance in our app with puma So far 1 agoo process is slightly slower than our clustered 4 puma processes 3692 req/s vs 4424 req/s

However agoo doesn't seem to launch in clustered mode. ./bin/agoo -p 8080 -w 4 -t 32

the above doesn't output any errors, just never gets to the line: listening on http://:8080.

however ./bin/agoo -p 8080 -t 32 does work

We run the app in a docker container: https://github.com/PlaceOS/auth/blob/agoo-server/Dockerfile

Any ideas?

ohler55 commented 2 years ago

I'll look into it this weekend. My unit tests worked last time I check so with any luck there is some configuration addition that is needed.

ohler55 commented 2 years ago

It worked for me on dedicated hardware, my laptop. Does ./bin/agoo -p 8080 -t 8 work on on one of you machines when not using docker?

A couple of points though. You will probably find that making the thread count too high actually decreases performance. The bottleneck on a single process is the Ruby global lock. Having more than somewhere around 8 just mean more context switching and threads tied up waiting. I usually do some benchmarking with an app and try to find the optimum number. Then increase the worker count.

stakach commented 2 years ago

Our app is IO bound so more threads mean we can do processing while waiting for IO to complete

I'm thinking the issue might be related to database connections and forking - I think puma forks before rails is loaded unless preload_app! is called and we're not calling that as it was too challenging to coordinate various gems initialisation

Which basically means we're sacrificing memory savings that can be gained with forking

ohler55 commented 2 years ago

IO bound outside Ruby I take it. Makes sense.

If the database connection is being made before forking I could see how that could be a problem. Agoo is independent of rails so I'm not sure when it gets initialized relative to Agoo. Depends on how it is started I suppose. I would be interested in exploring options that would make it work for you if you want to pursue this. I think the next step might be instrumenting the code with some print statements to figure out when database connections are made and when forking occurs. See if your theory is correct and after that figure out what can be added to Agoo to allow the database connections to be made after forking.

frvade commented 11 months ago

Hi there! I came across from the web page benchmark page (congrats with being the fastest in ruby). Running the Rails app in clustered mode would require to have at least before_fork hook as normally this is the place to close any connections that would be corrupted in the parent on child process exit. Like that

before_fork do
  Sequel::DATABASES.each(&:disconnect)
end

Also it would be nice to have some kind of an after_worker_fork callback to have the opportunity to do some work in the context of new spawned worker

ohler55 commented 11 months ago

Both sound like good ideas. Look for those additions in the near future.

ohler55 commented 9 months ago

A fork_wrap option was added in the develop branch. Please give it a try.

ohler55 commented 9 months ago

No response, closing.

xenyal commented 8 months ago

Hi @ohler55!

I was wondering if it might be possible to differentiate between when a worker is forked vs. when a worker is booted - I'm trying to migrate from Puma to Agoo, and in my containerized Rails environment, we have the following:

before_fork do
  ActiveRecord::Base.connection_pool.disconnect! if defined?(ActiveRecord)
end

on_worker_boot do
  ActiveRecord::Base.establish_connection if defined?(ActiveRecord)
end

Do you have any examples on how that can be migrated to use the new fork_wrap option? Thanks!

ohler55 commented 8 months ago

What does it mean to have a worker booted versus the app being forked?

xenyal commented 8 months ago

Based on the puma documentation:

before_fork - If you are preloading your application and using Active Record, it's recommended that you close any connections to the database before workers are forked to prevent connection leakage.

on_worker_boot - The code in the on_worker_boot will be called if you are using clustered mode by specifying a number of workers. After each worker process is booted, this block will be run. If you are using the preload_app! option, you will want to use this block to reconnect to any threads or connections that may have been created at application boot, as Ruby cannot share connections between processes.

So both options seem to account for the possibility of the app being preloaded when running in clustered mode, where the app boot and code loading happens before the workers are forked, as a means to reduce memory footprint - however without setting the on_worker_boot, leaves the possibility of threads not being connected to the database yet

xenyal commented 8 months ago

Since we wouldn't be preloading the app with agoo, I guess the on_worker_boot doesn't matter - we would only need to ensure that ActiveRecord is disconnected before_fork, and that it's connected after_fork between the master and worker processes: https://devcenter.heroku.com/articles/concurrency-and-database-connections#multi-process-servers - can that be achieved in the fork_wrap option?

ohler55 commented 8 months ago

If a forker object is set the before method will be called before forking and the after will be called after forking. I think that should work for you, yes?

xenyal commented 8 months ago

@ohler55 that would be perfect!

ohler55 commented 8 months ago

Right, that is the current behavior.

xenyal commented 8 months ago

Where might the before block be defined for agoo's config if running as part of Rails through rackup?

ohler55 commented 8 months ago

Look at example/quux.rb.