Add handling of timeouts to ApiWebRequest - #9144
Conversation
ca71479 to
89b5894
Compare
ApiWebRequestApiWebRequest
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89b58945db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenchmarksBenchmark execution time: 2026-08-28 16:21:03 Comparing candidate commit 01a2e42 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
8c9cb73 to
388d2f2
Compare
89b5894 to
c221aa9
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9144) and master. ✅ No regressions detected |
388d2f2 to
db88bf0
Compare
c221aa9 to
0e63bc8
Compare
There was a race where the Abort() callback could file shortly after returning a "success" response
…eadline Without this, you can cause a request to hang forever if the write body hangs, and can also fail to unblock a hanging GetResponseAsync(). Make it safe.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary of changes
Fixes timeout handling for
ApiWebRequestReason for change
Basically, timeouts never worked in our
ApiWebRequest, because the APIs we call (GetRequestStreamAsync/GetResponseAsync) simply don't pay attention to it. That means we effectively had no timeout on any http calls for .NET Framework.Implementation details
Actually handle a timeout in
ApiWebRequest. Technically this still isn't quite the same as ourHttpClientimplementations, because forHttpClientwe're using a mode that buffers everything in memory immediately, and applies the timeout including that. That's more effort to do withApiWebRequest, as you need to explicitly pass the cancellation token through to the response etc, which all becomes kinda different toHttpClient. Ultimately, what this adds is better than what we have todayNote: My assumption is that we will not want to buffer everything in memory soon, seeing as we're always explicitly copying the body or stream decoding it. Today, we are doing more copying than we should. I expect we'll look at using
HttpCompletionOption.ResponseHeadersReadinstead soon, in which case, we will need to unify our handling of timeouts in any case. A follow up process.Test coverage
Added unit tests for the timeouts (which hopefully are not flaky), given I'm cheating by passing in a deterministic cancellation token instead of trying to "wait for it to fail". There's a bunch of edge cases around exactly when the timeout is cancelled which we handle. There's the occasional additional edge case that we can't test without a lot more work, so left that to manual testing instead
Other details
Stacked on:
ApiWebRequest#9145Discovered as part of: