Skip to content
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

fix(h2_connection_reset): add stream reset when client cancel the req… #944

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MarkLux
Copy link

@MarkLux MarkLux commented Aug 19, 2024

Summary

add the missing connection reset of h2 connection when client cancel the request.

fix #943

in discussion with #941

@MarkLux MarkLux force-pushed the fix/h2_connection_reset branch from 7455935 to 9924db4 Compare August 20, 2024 03:28
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, seems like a good start. Make sure to run scripts/unasync.py to also apply the changes to the httpcore/_sync/http2.py module.

https://github.com/encode/httpcore/actions/runs/10464774808/job/28978912844#step:5:26

(Related... maybe we could change the build logs when unasync --check fails, so that there's clearer feedback to the developer.)

@MarkLux MarkLux force-pushed the fix/h2_connection_reset branch 4 times, most recently from 46031e9 to 5997901 Compare September 4, 2024 06:41
httpcore/_async/http2.py Outdated Show resolved Hide resolved
@MarkLux MarkLux force-pushed the fix/h2_connection_reset branch 3 times, most recently from af2e7e4 to c061e4f Compare September 4, 2024 08:15
@MarkLux MarkLux force-pushed the fix/h2_connection_reset branch from bd4f039 to 18422ff Compare September 4, 2024 09:45
@MarkLux
Copy link
Author

MarkLux commented Sep 11, 2024

sorry to bother but we are really in a hurry to fix this one, could you have a review again? @tomchristie

@MarkLux MarkLux requested a review from tomchristie September 27, 2024 03:55
@MarkLux
Copy link
Author

MarkLux commented Oct 12, 2024

langchain-ai#5

approved by langchain community now, shall we have any further progress on this one? @tomchristie

Comment on lines +578 to +579
# need send cancel frame when the exception is not from remote peer.
if not isinstance(exc, RemoteProtocolError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need send cancel frame when the exception is not from remote peer.

It's not obvious to me that not isinstance(exc, RemoteProtocolError) is correct/sufficient here.

Should this instead be something like just wrapping the yield?...

try:
    yield chunk
except Exception as exc:
    self._connection._reset_steam(
        stream_id=self._stream_id,
        error_code=h2.settings.ErrorCodes.CANCEL,
    )
    raise exc

@tomchristie
Copy link
Member

approved by langchain community now, shall we have any further progress on this one?

Thanks @MarkLux - apologies for the delay it's a bit of a complex one to review.

Thoughts on my review comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing client stream reset in h2 connection
2 participants