Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-69282: test_httpservers hangs since Python 3.5#9564
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
@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.
@csabella I made the requested changes. |
This is ready for review. |
This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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. |
Do we close this or do we remove the pending label? |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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'}) |
There was a problem hiding this comment.
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. |
Uh oh!
There was an error while loading.Please reload this page.
Added the following headers to RequestHandlerLoggingTestCase.test_get HTTP client request: Connection (value; close), Content-Length (value: 0)
This is a remake of the PR due to screw up I made in the last one.
https://bugs.python.org/issue25095