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

Stop browser opening consuming all the sockets #136

Merged
merged 1 commit into from Jul 5, 2015

Conversation

markbt
Copy link
Contributor

@markbt markbt commented Jul 4, 2015

The sense of the test in is_server_running is inverted.

This means wait_for_server races with the server starting:

  • If the server starts first, wait_for_server will never return, and will quickly consume all available ephemeral ports on the system.
  • If the server doesn't start first, wait_for_server returns immediately and the browser opens to a blank page.

This fix:

  • Fixes the sense of the test, and makes the code more explicit
  • Adds a delay in wait_for_server so that ephemeral sockets aren't rapidly consumed while we wait for it to start.

@joeyespo
Copy link
Owner

joeyespo commented Jul 5, 2015

Beat me to it. Glad to see you came to the same conclusion with the logic inversion and adding a sleep delay.

I was also wondering if adding sock.close() was necessary here if rc == 0. I was still seeing some strange behavior, however I usually run Grip with Werkzeug's debug=True, so that might be causing some other side effects with this. Have any thoughts about sock.close()?

Thanks for the fix, @markbt!

joeyespo added a commit that referenced this pull request Jul 5, 2015
Stop browser opening consuming all the sockets
@joeyespo joeyespo merged commit 7071625 into joeyespo:master Jul 5, 2015
@markbt
Copy link
Contributor Author

markbt commented Jul 5, 2015

The socket should get closed when sock goes out of scope and gets garbage collected. It certainly couldn't do any harm, though.

@joeyespo joeyespo added the bug label Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants