gh-69282: test_httpservers hangs since Python 3.5 by WildCard65 · Pull Request #9564 · python/cpython (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
@vadmium what do you think about this PR?
@WildCard65, please address the comments that @vadmium left on the bug tracker for this. Thank you!
Added 'self.con.close()' after 'self.con.getresponse()' to close the socket connection. Kept the 'Connection' header in the request to signal our intent to terminate the socket.
This is ready for review.
This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.
bpo-25095: test_httpservers hangs since Python 3.5 gh-69282: test_httpservers hangs since Python 3.5
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch seems fine and addresses all the points raised in #69282.
A
This issue and PR are no longer needed in that test_httpservers runs fine since 3.9. If some coredev familiar enough with httpserver thinks that the changes are a good idea anyway, please merge.
The issue will be closed if no feedback is provided
label
Do we close this or do we remove the pending label?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current proposed changes should fix the firewall problem as I understand it. The changes are a bit clumsy, but overall beneficial.
I don’t see any reason why the problem would already be fixed. Maybe @terryjreedy it’s just that you originally had a firewall interfering, but it isn’t any more?
| self.send_response(HTTPStatus.OK) |
|---|
| self.send_header('Content-Length', 0) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nitpick) It’s not really documented, and seems to work regardless, but I assume the header field value should be a string '0' not an integer
| # To combat this, we send the test server the "Connection" header |
|---|
| # with "close" for the value forcing the server and client to |
| # terminate the socket allowing the test to resume. |
| self.con.request('GET', '/', headers={'Connection': 'close'}) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation seems vague. I would explain it as:
“Connection: close” causes the server to set its “close_connection” flag. If the client did not also shut down the connection below, this flag would avoid a deadlock between the server waiting for a second request on the connection, and the main thread shutting the server down.
I have no idea why the test failed and then passed again on my machine. It works now as is on a debug no-gil build. I do know that code can be improved, and maybe made more stable, even if not failing on a particular machine at a particular time. Please judge on that basis. I also know that tests that pass on GH CI machines may fail on my machine, and vice versa, and need revision to run better on both and others.
This PR is stale because it has been open for 30 days with no activity.