Closed Marcus10110 closed 4 years ago
I probably should've noticed too, but don't have Windows easily handy to test on :/
This used to use https://docs.python.org/3/library/os.html#os.startfile, which (perhaps significantly) returns as soon as the associated application is launched
. It doesn't look like there's a way to pass arguments to that however, so moving to subprocess probably makes sense. Does the default timeout value need to increase, or is 5 seconds still working fine?
Does the default timeout value need to increase, or is 5 seconds still working fine?
Ironically, the default 5 seconds isn't long enough, but the socket still connects with 3 seconds to spare! Turns out that sock.connect_ex(('localhost', 10429))
takes 2 seconds to detect a connection failure, which was not accounted for in the loop.
In reality, it takes about 6 seconds for the socket to open and accept connections on Windows, and that's on a computer with a pretty fast SSD.
Without any changes, the code will probably allow up to 8 seconds for a connection, which is pretty tight. I've updated this PR to set the default timeout to 15 seconds, and I've changed the way it's enforced to use wall clock time. I've also added a return value to the function, it will return True when the socket was able to connect within the timeout.
🤦 My last PR to add command line args for launching logic used
os.system(p)
, which blocks until Logic exits. Not sure how I could missed that, I could have swore I tested it several times.This changes it to
subprocess.Popen