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

Close request when initial response fails and propagate error #454

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

alextwoods
Copy link
Contributor

Description of changes:
This fixes an issue where event stream initial response failures cause the stream to hang. There are two issues:

  1. The response body will not be closed until the request body gets closed (because the h2 streams will be left open). To signal the server to close the connection, we need to also close the request body to end it. This is accomplished by checking for error status codes and setting the response future to an exception. If the response is not closed, the deserialize_error methods will hang forever trying to read the body.
  2. We were incorrectly not calling the done method on the asyncio response_future and so were not setting the exception in it correclty.

There may be better ways to ensure the request body gets closed on an error in initial request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment on lines 199 to 207
def __init__(self, response: AWSCRTHTTPResponse) -> None:
self._status = response.status
self._response = response

super().__init__(f"Request failed with status code {self._status}")

@property
def response(self) -> AWSCRTHTTPResponse:
return self._response
Copy link
Contributor

Choose a reason for hiding this comment

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

What actually gets surfaced to the end user with the stack trace here? Is it only the message being passed to the base Exception constructor? Ideally we have more context if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exception gets caught in the AWSCRTHTTPClient.send method, which then closes the request body and returns the response upwards. The client code then deserializes the response using its deserializers and will create the appropriate service exception. An example for an expired security token:

Traceback (most recent call last):
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/client.py", line 595, in _execute_operation
    return await self._handle_execution(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/client.py", line 751, in _handle_execution
    return await self._finalize_execution(interceptor_chain, output_context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/client.py", line 1003, in _finalize_execution
    raise context.response
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/client.py", line 717, in _handle_execution
    raise output_context.response
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/client.py", line 916, in _handle_attempt
    output = await deserialize(response_context.transport_response, config)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexwoo/python/test-lexruntime/lexruntime/deserialize.py", line 403, in _deserialize_start_conversation
    raise e
lexruntime.models.UnknownApiError: ExpiredTokenException: The security token included in the request is expired

@alextwoods alextwoods marked this pull request as ready for review March 19, 2025 18:33
@alextwoods alextwoods requested a review from a team as a code owner March 19, 2025 18:33
@alextwoods alextwoods merged commit e1ecd23 into smithy-lang:develop Mar 19, 2025
2 checks passed
@alextwoods alextwoods deleted the fix_initial_error_hang branch March 19, 2025 22:40
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.

3 participants