-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
curl_httpclient: Future.cancel() closes connection #2728
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution, and I apologize for being so slow to respond. However, I think there are issues with performance that need to be addressed before this capability can be merged.
Also, new functionality like this needs a test, and I'd really like to have it implemented for both HTTP clients instead of being curl-only.
@@ -211,6 +212,12 @@ def _finish_pending_requests(self) -> None: | |||
self._finish(curl, errnum, errmsg) | |||
if num_q == 0: | |||
break | |||
for curl in self._curls: |
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.
Looping over every pending request each time any request finishes makes the entire thing quadratic, which isn't good. In order to support cancellation properly we need to make it event-driven: add a callback to the future that fires when it is cancelled, instead of periodically calling cancelled()
to check the status. (I think there might be more idiomatic ways to implement this for simple_httpclient which is already coroutine-based)
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.
All great feedback, thanks! Was desperate for a solution and had just learned about co-routines hours before writing this. I noticed some checks failed. Any insight on that? I did a somewhat deep dive, but was having trouble understanding what the errors even meant.
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.
Looks like the main test failure is just:
ERROR: test_error_after_cancel (tornado.test.curl_httpclient_test.CurlHTTPClientCommonTestCase)
...
Exception: did not get expected log message
That test is pretty straightforward:
def test_error_after_cancel(self):
fut = self.http_client.fetch(self.get_url("/404"))
self.assertTrue(fut.cancel())
with ExpectLog(app_log, "Exception after Future was cancelled") as el:
The other failing bits are formatting checks from flake8 and black
The user just needs to cancel the Future passed by the fetch() function, and it'll properly close the connection. This works on curl_httpclient. Simple_httpclient hasn't been implemented.