Repository navigation
[core] Support opt-in Expect: 100-continue in Node HTTP/1 transport - #40258
Jeremy Meng (jeremymeng) wants to merge 21 commits into
Conversation
Defer upload sources until continue or the fixed flush-based fallback, preserve early responses, and retire incomplete outgoing framing. Integrate lazy multipart factories, one-shot replay protection, redirects, and streamed legacy cancellation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f15004e9-d812-46bf-abcf-2a9bb184fcae
|
Azure Pipelines: Successfully started running 2 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Use the absolute repository URL required by Verify Links. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f15004e9-d812-46bf-abcf-2a9bb184fcae
|
/azp run js - storage-blob - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run js - storage-file-share - tests |
|
/azp run js - storage-file-datalake - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed stream-lifecycle regressions block approval, and the shared transport changes warrant final human review.
2 open findings
What changed in this PR
Adds opt-in Node HTTP/1 upload negotiation to the shared runtime and integrates it with core-rest-pipeline.
Changes:
- Defers uploads until HTTP 100 or a fixed fallback.
- Adds lazy multipart handling, replay protection, and response cancellation cleanup.
- Adds regression coverage, documentation, and coordinated package metadata updates.
| File | Description |
|---|---|
| sdk/core/ts-http-runtime/test/snippets.spec.ts | Adds negotiation example. |
| sdk/core/ts-http-runtime/test/public/node/expectContinueState.spec.ts | Tests negotiation state transitions. |
| sdk/core/ts-http-runtime/test/public/node/expectContinue.spec.ts | Tests wire behavior and lifecycle. |
| sdk/core/ts-http-runtime/test/internal/node/httpServer.ts | Adds loopback server and proxy helpers. |
| sdk/core/ts-http-runtime/test/internal/node/concat.spec.ts | Tests lazy multipart streams. |
| sdk/core/ts-http-runtime/test/internal/expectContinue.spec.ts | Tests effective header handling. |
| sdk/core/ts-http-runtime/src/util/nodeBody.ts | Tracks stream ownership and replay. |
| sdk/core/ts-http-runtime/src/util/expectContinue.ts | Handles negotiation headers. |
| sdk/core/ts-http-runtime/src/util/concat.ts | Makes multipart consumption lazy. |
| sdk/core/ts-http-runtime/src/policies/redirectPolicy.ts | Clears negotiation on body removal. |
| sdk/core/ts-http-runtime/src/policies/multipartPolicy.ts | Preserves factory size metadata. |
| sdk/core/ts-http-runtime/src/nodeHttpClient.ts | Implements negotiation and lifecycle handling. |
| sdk/core/ts-http-runtime/README.md | Documents negotiation behavior. |
| sdk/core/ts-http-runtime/CHANGELOG.md | Records runtime support. |
| sdk/core/core-rest-pipeline/test/public/node/expectContinue.spec.ts | Tests core integration. |
| sdk/core/core-rest-pipeline/src/util/file.ts | Retains lazy file factories. |
| sdk/core/core-rest-pipeline/src/util/abortSignalCleanup.ts | Defers response-stream cleanup. |
| sdk/core/core-rest-pipeline/src/policies/wrapAbortSignalLikePolicy.ts | Extends legacy cancellation forwarding. |
| sdk/core/core-rest-pipeline/src/policies/multipartPolicy.ts | Passes lazy factories to runtime. |
| sdk/core/core-rest-pipeline/src/defaultHttpClient.ts | Preserves streamed-response cancellation. |
| sdk/core/core-rest-pipeline/src/constants.ts | Updates SDK version. |
| sdk/core/core-rest-pipeline/README.md | Documents integration behavior. |
| sdk/core/core-rest-pipeline/package.json | Updates version and runtime minimum. |
| sdk/core/core-rest-pipeline/CHANGELOG.md | Records integration changes. |
| pnpm-lock.yaml | Updates dependency resolution metadata. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
🧠 Review effort: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
| import { logger as coreLogger } from "../log.js"; | ||
| import type { RestError } from "../restError.js"; | ||
| import { isRestError } from "../restError.js"; | ||
| import { disposeResponseBody } from "../util/responseBody.js"; |
There was a problem hiding this comment.
Copilot
: disposeResponseBody is needed when a policy receives one response but decides to send another request instead of returning that response—retry, redirect, or authentication challenge.
With streamed responses, ignoring the old body leaves its connection occupied. For an early final response during an unfinished upload, the socket is intentionally retired only when the response body closes. If the policy neither reads nor cancels that body, a one-socket agent can deadlock: the replacement request waits forever for the previous socket.
So these policies dispose the abandoned response before resending:
• Retry: before another attempt.
• Redirect: before following Location .
• Authentication: before resending after a challenge.
Disposal destroys/resumes Node streams or cancels browser streams. It is best-effort: cleanup failures are logged but must not replace the selected retry or resend. Responses returned to callers are not disposed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
|
/azp run js - storage-file-datalake - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run js - storage-file-share - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run js - storage-blob - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: feef5cbc-ea2d-48f7-916c-c481f4742476
* Simplify Expect: 100-continue implementation - Share disposeResponseBody from ts-http-runtime internal/util instead of duplicating it in core-rest-pipeline - Extract header preparation from NodeHttpClient.makeRequest into prepareExpectContinueHeaders - Deduplicate raw header pair normalization and body header stripping in expectContinue - Add shared toError and toRequestError helpers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1aa4c5c1-7005-4d8a-b7eb-2f63b8487ab2
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f7f163d1-8382-4cbb-adc7-eeb36717d037
|
/azp run js - storage-blob - tests |
|
/azp run js - storage-file-share - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run js - storage-file-datalake - tests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| if (readableStream.destroy) { | ||
| readableStream.destroy(); | ||
| } else { | ||
| readableStream.resume(); | ||
| } |
| if (!stream || !stream.readable) { | ||
| cleanup(); | ||
| return; |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44d4b275-f5cc-4ad3-ada9-7ca9ad006dc6
| const readableStream: unknown = response.readableStreamBody; | ||
| if (isNodeResponseBody(readableStream)) { | ||
| if (readableStream.destroy) { | ||
| readableStream.destroy(); |


Copilot agent
(on behalf of Jeremy Meng (@jeremymeng)):
Packages impacted by this PR
@typespec/ts-http-runtime(existing unreleased 0.3.10).@azure/core-rest-pipeline(tooling-managed patch increment to 1.25.1; runtime minimum updated to^0.3.10). Publish the runtime before or together with the core package.Issues associated with this PR
Fixes #40251.
Fixes #40387.
Describe the problem that is addressed by this PR
The built-in Node transport starts body factories, stream consumption, and upload progress before a server can decide whether to accept a potentially large upload. It also needs to preserve existing HTTP/1.1 streaming behavior, retries, redirects, authentication challenges, and caller-owned stream semantics when an early response arrives.
This PR adds opt-in
Expect: 100-continuesupport when the caller explicitly supplies the header. It does not automatically addExpector introduce a public option.Changes in this PR:
Hostgeneration when omitted, and derivesContent-Lengthor chunked framing beforeClientRequestconstruction..pipe()drain before resuming a multipart part or invoking the next factory, preserves readable-mode backpressure, and rejects every previously started, ended, closed, or disturbed one-shot source withREQUEST_BODY_NOT_REPLAYABLE.@azure/core-rest-pipelineto consume the new runtime version.HTTP 417 is returned normally before or after upload starts; there is no automatic 417 recovery. This PR does not add HTTP/2/Undici support, a configurable continue timeout, or transport-specific behavior to Fetch/XHR/React Native.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
ClientRequeststate.Expectheader write: withholdingExpectduring construction and restoring it after agent header rewrites supports forward proxies and custom agents.flushHeaders()provides a practical fallback boundary across fresh/reused connections, delayed DNS/TLS, HTTP forward proxies, and HTTPS CONNECT. It does not claim that the remote server has received the headers.Are there test cases added in this PR? (If not, why?)
Yes. Coverage includes:
createFileFromStreamfactories; readable-mode backpressure;.pipe()pause/drain with single- and multi-chunk parts; direct/factory ownership; replay protection across negotiated and non-negotiated attempts.Tests use ephemeral loopback endpoints and existing trusted test certificates and require no Azure service. Linux/macOS and minimum-Node coverage remain part of CI. Already-flowing sources and bytes queued inside Node cannot be made lazy or unsent retroactively.