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

Ability to optionally drop all connections after fork #177

Merged
merged 1 commit into from May 19, 2023

Conversation

shayonj
Copy link
Contributor

@shayonj shayonj commented May 18, 2023

There was a recent feature to automatically drop all connections after fork. This is quite nice and makes sense.

However, for some rails app that usually don't follow the fork model (like w/ unicorn/puma), and additionally have some logic to fork processes to perform internal business logic that doesn't rely or use ConnectionPool, the application can observe Redis connection issues or resets. These forks can happen during application run time. Like ours.

In such a case, it'd be nice to not automatically drop all the connections, since the underlying process isn't working with Redis/ConnectionPool, and as a sideeffect the pool in the primary process is impacted.

This PR proposes a new attribute auto_reload_after_fork as a config option. By default it is true. However, application users can turn it to false and not opt in for the feature to auto drop connections after fork.

This could be quite useful for us

Closes: #175

@shayonj shayonj force-pushed the s/optional-fork branch 2 times, most recently from 57d4a65 to 7ad6359 Compare May 18, 2023 13:23
There was a recent feature to automatically drop all connections after fork. This is quite nice
and makes sense.

However, for some rails app that usually follow the fork model (like w/ unicorn/puma), and additionally
have some logic to fork processes to perform internal business logic that doesn't rely or use ConnectionPool,
the application can observe Redis connection issues or resets. These forks can happen during application run time.
Like ours.

In such a case, it'd be nice to not automatically drop all the connections, since the underlying process isn't working
with Redis/ConnectionPool, and as a sideeffect the pool in the primary process is impacted.

This PR proposes a new attribute auto_reload_after_fork as a config option. By default it is true. However, application
users can turn it to false and not opt in for the feature to auto drop connections after fork.

This could be quite useful for us
@shayonj shayonj changed the title Ability to optioanlly drop all connections after fork Ability to optionally drop all connections after fork May 18, 2023
@mperham
Copy link
Owner

mperham commented May 19, 2023

Seems fine to me; I can see how this would be useful for some pools.

@mperham mperham merged commit f7463bb into mperham:main May 19, 2023
8 checks passed
@shayonj
Copy link
Contributor Author

shayonj commented May 19, 2023

Thanks for the fast review and release 🙌🏾

@shayonj shayonj deleted the s/optional-fork branch May 19, 2023 18:25
@KJTsanaktsidis
Copy link

as a sideeffect the pool in the primary process is impacted.

This sounded strange to me when I first read it, but it happened to us too and I figured out why - it happens if the pool is managing TLS connections to something (Redis in our case, but anything using Ruby's openssl would work the same)

Calling OpenSSL::SSL::SSLSocket#close calls SSL_shutdown on the openssl context, which results in sending a TLS "close notify" packet down the wire, which will prompt the remote side to send a TCP FIN and close the connection; that then is what affects the parent process (it sees the connection as closed underneath it from the remote side).

In our main unicorn boot process, we actually close all the connections before forking, so this isn't an issue there, but we discovered this issue because there was some unexpected fork happening inside our Rails controllers!

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.

Connection resets and connection timeouts on threads after fork
3 participants