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

Prevent server crash by restarting child #727

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

prognostikos
Copy link

Before this change, if Errno::EINVAL is thrown when sending IO to the child the server would crash. Now we catch the exception and, assuming the child has problems, we start a new child.

Before this change, if Errno::EINVAL is thrown when sending IO to the
child the server would crash. Now we catch the exception and, assuming
the child has problems, we start a new child.
@prognostikos
Copy link
Author

prognostikos commented Sep 12, 2024

This is a solution, or maybe better characterized as a work-around, for #716

Before this change, in a Rails 7.0.8.4 app using ruby 3.3.4 (2024-07-09 revision be1089c8ec) [arm64-darwin23] I could trigger the error as follows:

  1. start spring using spring server in one terminal
  2. start rails c in a second terminal
  3. run rails db:migrate:status in a third terminal - it succeeds
  4. run touch config/initializers/content_security_policy.rb in the third terminal
  5. run rails db:migrate:status in the third terminal - it hangs with no output and the console running in the second terminal is dead.
  6. in the first terminal, spring dies with lib/spring/application_manager.rb:64:in `send_io': Invalid argument - sendmsg(2) (Errno::EINVAL)

I have put the spring server logs for both the error and fixed versions here: https://gist.github.com/prognostikos/0d07e599516e51ddc355134e13c94a95

I would love to get this into a new Spring release, what needs to be done for that to happen? I can see there are some issues with CI, and running the acceptance tests locally with RAILS_VERSION="edge" also seems to hang so there is probably a bit of work to do on the test setup.

@prognostikos prognostikos marked this pull request as ready for review September 12, 2024 10:37
@@ -42,7 +42,7 @@ def with_child
if alive?
begin
yield
rescue Errno::ECONNRESET, Errno::EPIPE
rescue Errno::ECONNRESET, Errno::EPIPE, Errno::EINVAL
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do some kind of cleanup here in case the child is hanging around as a zombie process?

@fschwahn
Copy link

This seems to fix the problems I encountered in #716 👍

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.

2 participants