Fix error "[Errno 111] Connection refused" and make logs more usable #1029
Conversation
MVrachev
changed the title
There was a problem hiding this comment.
Thanks for the solid changes to fix this issue @MVrachev.
A few minor suggestions and this is good to go:
- We only need the change to invoke
simple_server.pywith cwd for tests/test_multiple_repositories_integration.py and tests/test_updater.py – could you restore the other tests to their current behaviour of just calling simple_server.py ? - I added some comments to simple_server.py itself to keep that code short and idiomatic, could you please address those?
Thanks
tests/simple_server.py
Outdated
| if len(sys.argv) > 2: | ||
| use_quiet_http_request_handler = sys.argv[2] | ||
|
|
||
| if use_quiet_http_request_handler == True: |
There was a problem hiding this comment.
As this is Python and we expect a boolean value we can just test the variable like:
| if use_quiet_http_request_handler == True: | |
| if use_quiet_http_request_handler: |
See PEP 8's Programming Recommendations and the Book of Python Anti-Patterns
There was a problem hiding this comment.
Yea, I just wanted to be more explicit, but you are right.
I will address this.
tests/simple_server.py
Outdated
| # server on those Windows/Py2 to not fill the buffer. | ||
| if six.PY2 and platform.system() == 'Windows': | ||
| handler = QuietHTTPRequestHandler |
There was a problem hiding this comment.
I don't think we still need this logic above if we now default to using QuietHTTPRequestHandler? Certainly we don't want two ways to change the HTTP Request Handler for this simple test server.
There was a problem hiding this comment.
I didn't want to remove this check so that this strange case for Windows is documented.
If I remove it now and someone change this piece of code again this bug can reappear.
There was a problem hiding this comment.
I can see the logic in that, we can either leave the comment and remove the code or consolidate this logic with your change. We should absolutely not set the handler in two places which can conflict with each other. As it stands if I explicitly set use_quiet_http_request_handler = False on Windows, I still get QuietHTTPRequestHandler.
There was a problem hiding this comment.
Yes, you are right. I will group those checks together so that we don't change the state in two places.
I am wondering which should be with higher priority: use_quiet_http_request_handler value or the check if we are on Windows?
I think that if we are on Windows should be with higher priority given that we have bug reported for that specific case.
No, these calls are restricted to test code and are to ensure that we have the tests/ directory in order to a) serve the test repo data and b) ensure we are invoking simple_server.py in a way which won't cause errors when there are multiple tests/ directories in the PYTHONPATH (as is the case when we're running with editable securesystemslib and editable tuf installations). |
Fixes issue: theupdateframework#1010 When running the tests this error appears "[Errno 111] Connection refused". After some digging Lukas found another error "No module named tests.simple_server" which is the root case for error 111. With the help of Joshua Lock, we found out that the simple_server.py was not being found because subprocess.Popen was being passed a cwd kwarg which moved the current working directory away from tuf/tests. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Passing a pipe to the subprocess, but not reading from it conceals helpful error messages. As the code redirects all of the stderr from the subprocess to nowhere, the error output of the process is never read. If we remove the PIPEs from the tests we should see some error messages on the console/logger that can help us understand what went wrong. On another hand, when we stop passing stderr=subprocess.PIPE arg to the subprocess.Popen function call there are a lot of HTTP messages together with the helpful error messages. One decision is to make QuietHTTPRequestHandler the default. That way we receive the helpful error messages without the HTTP messages. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Yes, you are right. I updated my pr with the above suggestions and the ones left alongside the code. |
After a discussion with Joshua Lock, we agreed that for Windows users it would be good to provide the option to use SimpleHTTPRequestHandler, but still leave a warning about it, knowing that this caused an error before. See: theupdateframework@7dbb30a Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Fixes issue #: #1010
Description of the changes being introduced by the pull request:
When running the tests this error appears
"[Errno 111] Connection refused". After some digging Lukas
found another error "No module named tests.simple_server"
which is the root cause for error 111.
With the help of Joshua Lock, we found out that the simple_server.py
was not being found because of subprocess.Popen was being passed
a cwd kwarg which moved the current working directory
away from tuf/tests.
That's what I fix with the first commit.
In my second commit, I remove stderr=subprocess.PIPE argument from subprocess.Popen function calls and make the QuietHTTPRequestHandler as the default HTTP handler in the
simple_server.pyscript.This change is inspired by the digging needed to discover the real problem causing issue 1010 summarized well in the @lukpueh comment #1010 (comment).
Please verify and check that the pull request fulfills the following
requirements: