-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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 request pool to improve delivery performance #10353
Conversation
c0d7bc9
to
1a0bc64
Compare
6ce5105
to
ac6e32a
Compare
Such a cool new feature! I've cherry-picked When the app is starting, I noticed a single occurrence of I'll keep the server running the code for a while and see what happens. |
From https://github.com/mperham/connection_pool
Both http.rb and our own |
@zunda I have noticed this too. It seems to disappear when I remove the monkey-patch of the The intent of the monkey-patch is to remove fixed size of the pool. If more threads try to access the same URL than the pool size, they will wait up to 5 seconds (and then fail), which would be a performance regression. While without fixed size, even if all those threads won't benefit from keep-alive on first call, performance will improve over repeated calls, and the reaper will remove unused connections over time. There should be no way that
@ThibG This reminds me of the warning not to use rack-timeout. I am not sure if this is truly a deal-breaker, or the timeouts are encapsulated enough not to break connection pooling. |
ac6e32a
to
3f8df86
Compare
I have resolved the |
Coool! I've merged 3f8df86 and confirmed that the error is gone. |
a03f225
to
43269ce
Compare
I could use some help debugging "too many open files" errors that seem to appear in this PR after some use |
b094f45
to
7823b8b
Compare
7823b8b
to
83e1b31
Compare
Testing in production reveals a lot of timeout errors due to full pools, so it's not ready yet. |
I rewrote the code so that if the maximum pool size is reached, and we try to get a connection for a host that we don't have idle connections ready for, it removes one of the idle connections for a different host and creates a new one in its place. |
* Add request pool to improve delivery performance Fix mastodon#7909 * Ensure connection is closed when exception interrupts execution * Remove Timeout#timeout from socket connection * Fix infinite retrial loop on HTTP::ConnectionError * Close sockets on failure, reduce idle time to 90 seconds * Add MAX_REQUEST_POOL_SIZE option to limit concurrent connections to the same server * Use a shared pool size, 512 by default, to stay below open file limit * Add some tests * Add more tests * Reduce MAX_IDLE_TIME from 90 to 30 seconds, reap every 30 seconds * Use a shared pool that returns preferred connection but re-purposes other ones when needed * Fix wrong connection being returned on subsequent calls within the same thread * Reduce mutex calls on flushes from 2 to 1 and add test for reaping
* Add request pool to improve delivery performance Fix mastodon#7909 * Ensure connection is closed when exception interrupts execution * Remove Timeout#timeout from socket connection * Fix infinite retrial loop on HTTP::ConnectionError * Close sockets on failure, reduce idle time to 90 seconds * Add MAX_REQUEST_POOL_SIZE option to limit concurrent connections to the same server * Use a shared pool size, 512 by default, to stay below open file limit * Add some tests * Add more tests * Reduce MAX_IDLE_TIME from 90 to 30 seconds, reap every 30 seconds * Use a shared pool that returns preferred connection but re-purposes other ones when needed * Fix wrong connection being returned on subsequent calls within the same thread * Reduce mutex calls on flushes from 2 to 1 and add test for reaping
Fix #7909
Use keep-alive connections to each site, in connection pools, to reduce DNS/TCP/SSL handshake overhead. Keep connections for 300 idle seconds, then clean them up.
Benchmark: 400
ActivityPub::DeliveryWorker
jobs addressed to the same inbox on 5 Sidekiq threads