Skip to content

Conversation

@SamRemis
Copy link
Contributor

@SamRemis SamRemis commented Nov 24, 2025

Description

This PR improves timeout error messages to provide more actionable information for debugging. Currently, timeout errors sometimes display messages like "Client timeout occurred: " with no additional context. This occurs when the error's string representation is an empty string.

For the CRT client, we updated _CRTTimeoutError to include the underlying AWS error name and message, changing errors from Client timeout occurred: to CRT AWS_IO_SOCKET_TIMEOUT: socket operation timed out.

For all clients, we now default to using the exception's class name when the string representation is empty. This can occur in aiohttp when the total attribute is set on the ClientTimeout class, which will raise a TimeoutError with no additional info. This updates the same blank error message to A timeout error occurred..

Test file:
timeout_exceptions_test.py

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

@SamRemis SamRemis marked this pull request as ready for review November 25, 2025 00:02
@SamRemis SamRemis requested a review from a team as a code owner November 25, 2025 00:02
@SamRemis SamRemis marked this pull request as draft November 25, 2025 00:08
@SamRemis
Copy link
Contributor Author

Setting back to draft mode for now; I wanted to get this out there to confirm everyone is good with the two-pronged approach of both updating our CRT errors and having a default.

I think the exception message should ultimately be something along the lines of ""Client timeout occurred: {error_msg}, please see underlying exception for more details.", but wanted to get team input on final messaging.

)
except Exception as e:
if isinstance(e, self.transport.TIMEOUT_EXCEPTIONS):
error_msg = str(e) or type(e).__name__
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like we're leaking a detail somewhere. Is this something coming from the CRT that's requiring us to pull the __name__ off the error?

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 is more meant to target cases like aiohttp's total_timeout which only raises an empty asyncio.TimeoutError with no other context. I put a link to an example of this in the PR's description. It's just a way for us to make sure there's some string representation in an error where the string representation of it is otherwise empty.

Example from a Python REPL:

>>> str(TimeoutError())
''

Copy link
Contributor

Choose a reason for hiding this comment

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

from should give us the context (and the originating error). If they don't include anything, that's fine. By creating the chain, we can look to see what happened. Including {e} in the message to begin with is likely not necessary. That's an artifact of a time before we had better tracebacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

A timeout error occurred. is probably sufficient in this case.

except AwsCrtError as e:
if e.name in self._TIMEOUT_ERROR_NAMES:
raise _CRTTimeoutError() from e
raise _CRTTimeoutError(f"CRT {e.name}: {e.message}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we should be keeping the context with the from but we can include the message to make the immediate information clearer.

@SamRemis SamRemis marked this pull request as ready for review December 3, 2025 23:55
@nateprewitt nateprewitt merged commit 12c3e0a into smithy-lang:develop Dec 5, 2025
4 checks passed
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.

2 participants