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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timeout when connecting to server #708

Merged

Conversation

technicalpickles
Copy link
Contributor

Over on #663 we've seen the spring server get into a funky state, where the server.gets just hangs indefinitely.

I found other places in client.rb that checked that there was something to read before reading, and made sure server.gets returns something. So, applied that to verify_server_version 馃榿

@technicalpickles
Copy link
Contributor Author

I tested this two ways:

  1. bin/spring start in one console. background it, and run bin/rails console (or any other binstub)
  2. socat unix-listen:tmp/spring.sock /dev/null in one console. set SPRING_PIDFILE=tmp/spring.pid SPRING_SOCKET=tmp/spring.sock, and add the socat pid to tmp/spring.sock

In both, the binstub raises an error as expected.

@casperisfine
Copy link

cc @byroot

@@ -114,7 +114,16 @@ def stop_server
end

def verify_server_version
server_version = server.gets.chomp
unless IO.select([server], [], [], CONNECT_TIMEOUT)
Copy link
Member

Choose a reason for hiding this comment

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

So note that this only tell us there's something to read, it doesn't guarantee it's a whole line, so we can still potentially get stuck in server.gets.

However it should catch a large portion of such issues, so I think it's a good improvement.

@byroot byroot merged commit fc30464 into rails:main Dec 7, 2023
18 checks passed
@technicalpickles technicalpickles deleted the timeout-around-verify-server-version branch December 7, 2023 17:40
@technicalpickles
Copy link
Contributor Author

Thank you! I'll be testing this on our app, and I'm hoping it will help surface when #663 happens

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.

None yet

3 participants