-
Notifications
You must be signed in to change notification settings - Fork 107
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
Handle asynchronous cancellations for HTTPConnection instances #802
Handle asynchronous cancellations for HTTPConnection instances #802
Conversation
@@ -94,7 +96,7 @@ async def handle_async_request(self, request: Request) -> Response: | |||
stream=stream, | |||
keepalive_expiry=self._keepalive_expiry, | |||
) | |||
except Exception as exc: | |||
except BaseException as exc: |
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 is to be sure that it works when we cancel the asynchronous task.
Co-authored-by: Zanie Blue <contact@zanie.dev>
Co-authored-by: Zanie Blue <contact@zanie.dev>
Alternatively, we can add states for HTTPConnection instances (NEW, CONNECTING, FAILED), but I believe we should avoid that because we already have connecting states in HTTP11Connection instances. |
This PR works well based on my testing. |
Now the connection has a state of connecting, even if it was just created and is not trying to connect somewhere. This pull request simply adds the new state for such connections. |
We're experiencing timeout errors in various integrations, but it does not look like we ever send the request to OS2mo/Keycloak/etc. This MR bumps httpx to bump httpcore to 0.18.0 to include this fix: encode/httpcore#641. If we're lucky, that's it. Otherwise, we are probably waiting for this to be merged: encode/httpcore#802.
We're experiencing timeout errors in various integrations, but it does not look like we ever send the request to OS2mo/Keycloak/etc. This MR bumps httpx to a patched version which in turn depends on patched version of httpcore which includes encode/httpcore#802, which I suspect is our issue. Another related issue has also been fixed (encode/httpcore#641), and is already included in httpcore 0.18.0.
Now #785 closed, I think you can close this PR too. |
Motivated by encode/httpx#947 🔥
Closes #785
This pull request just adds some logic to check if the connection was previously used or if it's just been created.
Now the example from #785 works fine(?).
It's very hard to explain where the problem actually is, but I think you can find the answer to that question in #785.
In simple words, the client always thinks that there cannot be a connection that has not failed and is also not in the connecting process. This is because every connection is attached to some request when it's created, and there are no cases when it can just hang in our connection pool except for asynchronous cancellations.
Now the
HTTPConnection
instances can be new if "._is_new" is set to true and failed if "._connect_failed" is set to true. Otherwise, if both of them are false, that means it's in the connecting process.