Repository navigation
Refactor ApiWebRequest - #9145
Merged
Merged
Conversation
andrewlock
added
area:tracer
Member
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c9cb733ef
ℹ️ 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-10-22 14:32:28 Comparing candidate commit 0e2c4e3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 71 metrics, 1 unstable metrics, 5 known flaky benchmarks, 121 flaky benchmarks without significant changes.
|
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
2 times, most recently
from
August 27, 2026 19:21
388d2f2 to
db88bf0
Compare
andrewlock
marked this pull request as ready for review
August 27, 2026 19:22
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9145) and master. ✅ No regressions detected |
dudikeleti
approved these changes
Sep 23, 2026
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
October 19, 2026 14:21
db88bf0 to
55df10d
Compare
…ce, we'll do it again.
HttpClient throws TaskCanceledException for timeouts, while HttpStreamRequest calls TimeoutException when it times out trying to connect. Both count as timeouts
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
October 22, 2026 09:38
55df10d to
8a2cfee
Compare
) ## Summary of changes Removes the opt-in `enforceAsyncTimeout` flag (and the unused `_asyncTimeout` plumbing) from `ApiWebRequestFactory`/`ApiWebRequest`, its callers (`AgentTransportStrategy`, `FeatureFlagsEvpTransport`, tests), and deletes `ApiWebRequestFactoryTests.AsyncDeadlineAbortsStalledJsonSendOnlyWhenEnabled`. ## Reason for change #9235 added an opt-in `Timer` + `Abort()` deadline for async `HttpWebRequest` sends. The refactor in #9145 replaces the send path, and #9144 enforces `HttpWebRequest.Timeout` for every async send, so the flag no longer does anything. The deleted test asserted behaviour specific to the opt-in design: it disables `HttpWebRequest.Timeout`, expects a separate deadline to fire anyway, and expects a `WebException(RequestCanceled)`. The previous implementation had multiple issues: - Body writer stuck on something other than the request stream would hang indefinitely - Race condition when response returned just as the timer fires, killing a response that's already returned - Orphaned GetResponseAsync that succeeds after the abort is not handled ## Implementation details Sits between #9145 (base) and #9144 in the stack. ## Test coverage The deadline behaviour stays covered by: - `ApiRequestTests`, added in #9144 - `FlagEvaluationTransportTests.SlowTcpAgentTimesOutWithoutReplayingBatch` - `ExposureApiTimeoutTests.LegacyExposureRequestsAbortStalledConnection` On their own, #9145 and this PR have no async deadline on .NET Framework, so `SlowTcpAgentTimesOutWithoutReplayingBatch` fails on net48 until #9144 is applied. With the full stack applied, all transport and Feature Flags timeout tests pass locally on net48 and net10.0. ## Other details Merge together with #9144. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Removing the abort timer lets stalled feature-flag requests hang indefinitely on .NET Framework, blocking subsequent exposure delivery despite the configured five-second timeout.
🤖 Bits Code Review · Commit 6f295f2
## Summary of changes Fixes timeout handling for `ApiWebRequest` ## Reason 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 our `HttpClient` implementations, because for `HttpClient` we're using a mode that buffers everything in memory immediately, and applies the timeout including that. That's more effort to do with `ApiWebRequest`, as you need to explicitly pass the cancellation token through to the _response_ etc, which all becomes kinda different to `HttpClient`. Ultimately, what this adds is better than what we have _today_ **Note**: 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.ResponseHeadersRead` instead 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: - #9145 Discovered as part of: - #9042 --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
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 of changes
Refactoring of
ApiWebRequestlogic in preparation for follow up PRReason for change
I needed to do some work to add timeouts in #9144, and that required doing some cleanup first. Extracted here for simplicity of reviews
Implementation details
Commits can most easily be reviewed individually:
OperationCancelledExceptionas well to know if a failure was a timeout (for telemetry purposes)ApiWebRequestby pushing through a centralSendAsync()#nullable enableTest coverage
Just a refactor, so basically covered by existing tests
Other details
Required by #9144