Use descriptive ErrorMessage for timed out requests#2355
Closed
alexeyzimarev wants to merge 2 commits intodevfrom
Closed
Use descriptive ErrorMessage for timed out requests#2355alexeyzimarev wants to merge 2 commits intodevfrom
alexeyzimarev wants to merge 2 commits intodevfrom
Conversation
… values to strings Previously, AddParameter<T>, AddQueryParameter<T>, AddUrlSegment<T>, AddHeader<T>, AddOrUpdateParameter<T>, AddOrUpdateHeader<T>, and Parameter.CreateParameter all used ToString() which respects the current culture. This caused issues on non-English locales where e.g. 1.234 would be formatted as "1,234" with a comma decimal separator. All generic overloads now default to InvariantCulture and accept an optional CultureInfo parameter for callers who need locale-specific formatting. Fixes #2270 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a request times out, ErrorMessage was set to the TaskCanceledException
message ("A task was canceled.") which contradicts ResponseStatus.TimedOut.
Now ErrorMessage says "The request timed out." for timeout scenarios, keeping
all three state properties (ResponseStatus, ErrorMessage, ErrorException)
consistent. ErrorException still holds the original TaskCanceledException.
Fixes #2257
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Review Summary by QodoFix timeout error message and parameter culture handling
WalkthroughsDescription• Use invariant culture for parameter value conversions to fix locale-specific formatting issues • Add optional culture parameter to generic parameter methods for locale-specific formatting when needed • Use descriptive error message "The request timed out." for timeout scenarios instead of "A task was canceled." • Ensure consistency between ResponseStatus.TimedOut, ErrorMessage, and ErrorException for timeout cases Diagramflowchart LR
A["Parameter Conversion"] -->|"Use InvariantCulture by default"| B["Consistent Formatting"]
C["Timeout Exception"] -->|"Detect OperationCanceledException"| D["Set ErrorMessage to timeout message"]
D -->|"Keep ErrorException unchanged"| E["Consistent State Properties"]
B --> F["Enhanced RestSharp"]
E --> F
File Changes1. src/RestSharp/Extensions/StringExtensions.cs
|
Contributor
Code Review by Qodo
1. Binary-breaking API signature change
|
|
Deploying restsharp with
|
| Latest commit: |
bf41159
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://62648670.restsharp.pages.dev |
| Branch Preview URL: | https://fix-timeout-error-message.restsharp.pages.dev |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+58
to
+59
| public RestRequest AddParameter<T>(string name, T value, bool encode = true, CultureInfo? culture = null) where T : struct | ||
| => request.AddParameter(name, value.ToStringValue(culture), encode); |
Contributor
There was a problem hiding this comment.
1. Binary-breaking api signature change 🐞 Bug ⛯ Reliability
Several public generic RestRequest extension methods were changed to add a CultureInfo parameter instead of adding new overloads. This breaks binary compatibility: consumers compiled against prior versions can hit MissingMethodException at runtime when loading the updated assembly.
Agent Prompt
## Issue description
Public generic extension methods changed signatures by adding a new `CultureInfo? culture` parameter. This is source-compatible but **not binary-compatible**: existing compiled clients expecting the old signatures may fail at runtime with `MissingMethodException`.
## Issue Context
These methods are public API surface. Optional parameters do not preserve binary compatibility when the parameter is newly added (signature changes).
## Fix Focus Areas
- src/RestSharp/Request/RestRequestExtensions.cs[58-81]
- src/RestSharp/Request/RestRequestExtensions.Headers.cs[54-77]
- src/RestSharp/Request/RestRequestExtensions.Query.cs[44-45]
- src/RestSharp/Request/RestRequestExtensions.Url.cs[44-45]
## Suggested implementation sketch
- Add back overloads with the old signatures, e.g.:
- `AddParameter<T>(string name, T value, bool encode = true)` calls `AddParameter(name, value, encode, culture: null)`.
- Keep the new overload with `CultureInfo? culture = null`.
- Repeat for other affected generic methods.
- (Optional) Mark the old overloads `[Obsolete]` if you want to guide users toward the new culture-aware overloads, but keep them for binary compatibility.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Test Results 42 files 42 suites 17m 53s ⏱️ Results for commit bf41159. |
Member
Author
|
Recreating from a clean branch based on dev. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ErrorMessagenow says"The request timed out."instead of"A task was canceled."ErrorExceptionstill holds the originalTaskCanceledExceptionunchangedResponseStatus.TimedOut+ meaningfulErrorMessage+ originalErrorExceptionFixes #2257
Test plan
🤖 Generated with Claude Code