feat(undici-http-handler): add HttpHandler backed by Node.js undici#2029
feat(undici-http-handler): add HttpHandler backed by Node.js undici#2029trivikr wants to merge 26 commits into
Conversation
1e62635 to
3c321e1
Compare
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
e87cecf to
450e88b
Compare
This reverts commit 3d71928. It's a one-way door to strictly depend on undici version. We can specify it in peerDependencies for example.
|
There are a lot of issues with monorepo setup which blocks us from using private fields in UndiciHttpHandler. |
This reverts commit ba0f0a4.
| "typedoc": "0.23.23" | ||
| }, | ||
| "peerDependencies": { | ||
| "undici": "^6.0.0" |
There was a problem hiding this comment.
This should only appear in one of deps and peerDeps, right?
There was a problem hiding this comment.
A module, when it appears in both dependencies and peerDependencies within a package.json file, it usually means a default fallback for the dependency.
When we specify undici@^6.0.0 in dependencies, that mean it's the default. When we specify undici@^6.0.0 in peerDepedencies, it means customers can use those versions when passing their custom dispatcher.
The handler as it's currently designed will accept 7.x and 8.x versions of undici too, if customers on newer versions of Node.js and want to avail performance benefits.
There was a problem hiding this comment.
I considered removing undici from dependencies and asking customers to provide a dispatcher.
But customers will have to import both the handler and undici, which won't be a good minimal experience.
Existing minimal code
import { S3 } from "@aws-sdk/client-s3";
import { UndiciHttpHandler } from "@smithy/undici-http-handler";
const client = new S3({
requestHandler: new UndiciHttpHandler(),
});Minimal code if we require customer to pass dispatcher
import { S3 } from "@aws-sdk/client-s3";
import { UndiciHttpHandler } from "@smithy/undici-http-handler";
import { Agent } from "undici";
const client = new S3({
requestHandler: new UndiciHttpHandler({ dispatcher: new Agent() }),
});I think the former is better.
There was a problem hiding this comment.
I see, so should the one in peerDep be stated as >=6.0.0 then?
| public destroy(): void { | ||
| if (this.config.dispatcher && !this.externalDispatcher) { | ||
| this.config.dispatcher.destroy(); | ||
| this.config.dispatcher = undefined; |
There was a problem hiding this comment.
what if the user intent/expectation is that the destroy method destroys their dispatcher too?
There was a problem hiding this comment.
By default, we should not destroy the dispatcher as it's external.
But I can also see that most of the customers would likely create dispatcher specifically for the client.
I would like to keep this behavior, and make a change in major version bump is customers request it.
Their existing code, which destroys the custom dispatcher would be backward compatible.
There was a problem hiding this comment.
update: I realized that we destroy the user provided agent in node-http-handler, so it would make sense to also do so here
ref:
smithy-typescript/packages/node-http-handler/src/node-http-handler.ts
Lines 131 to 134 in 396de9c
| } | ||
|
|
||
| public httpHandlerConfigs(): UndiciHttpHandlerOptions { | ||
| return { ...this.config }; |
There was a problem hiding this comment.
why copy here? in node http2 I see return this.config ?? {}
There was a problem hiding this comment.
If we don't copy, the callee can mutate dispatcher/logger directly and can bypass the intended runtime encapsulation. We should update node http2 handler implementation.
There was a problem hiding this comment.
should the return type be marked Readonly<UndiciHttpHandlerOptions>?
| const contents = fs.readFileSync(file); | ||
|
|
||
| if (file.endsWith(".spec.ts")) { | ||
| if (file.endsWith(".spec.ts") || file.endsWith(".bench.ts")) { |
There was a problem hiding this comment.
if we move the bench to /tests it wouldn't need this exception, as only src is walked.
There was a problem hiding this comment.
This should be okay, since .bench.ts is a recommended standard for naming benchmark files in vitest.
And we're likely going to use the recommendation for future benchmarks.
Historically, we've use test folder only for clients as we overwrite the src folder which is code generated. For non-clients modules, we tend to keep tests as close to the source as possible.
I'm open to naming benchmark files as *.bench.spec.ts, but that would be against the recommendation from vitest. Do you have a preference?
There was a problem hiding this comment.
leaving .bench.ts here is fine, is there some way to globally exclude the pattern from compilation into dist-cjs/es/types?
2ac220f to
8c3a032
Compare
| // Uses the same duck-typing check as undici's isStream (pipe + on). | ||
| const body = request.body as Readable | undefined; | ||
| if (body && typeof body.pipe === "function" && typeof body.on === "function") { | ||
| if (headers["transfer-encoding"] === "chunked") delete headers["transfer-encoding"]; |
There was a problem hiding this comment.
http headers are case insensitive entirely, things like EXPECT and Transfer-encoding will slip through here, is it ok?
| } | ||
| } | ||
| if (request.fragment) { | ||
| path += `#${request.fragment}`; |
There was a problem hiding this comment.
does this need to be appended and sent over the wire? reading some RFCs around this here (9110, 3986, 9112) it seems like the server could reject it or ignore it.
Some protocol elements that refer to a URI allow inclusion of a fragment, while others do not.
Note: The fragment identifier component is not part of the scheme definition for a URI scheme....
and it does not seem to be a part of the request-targets either (RFC 9112)
| for (const key in responseHeaders) { | ||
| const value = responseHeaders[key]; | ||
| if (value !== undefined) { | ||
| transformedHeaders[key] = Array.isArray(value) ? value.join(", ") : value; |
There was a problem hiding this comment.
does this transformation need to be unconditional? we might have some perf gains here: most headers might not require this transformation, they should be strings. We might not need to make a new allocation and x iterations
we could check if the value is indeed an array otherwise skip this copying. i'm hoping the multi-value-header case is indeed rare in which case this could be optimized further
Issue #, if available:
Migrating code from test package in https://github.com/trivikr/trivikr-test-undici-http-handler
Description of changes:
Adds a new
@smithy/undici-http-handlerpackage that provides a Smithy-compatible HTTP handler backed by undici instead of Node.js native http/https modules.undici offers improved HTTP performance over Node.js built-in modules. Benchmarks show 20%-30% less time spent in request handling compared to
NodeHttpHandlerfrom@smithy/node-http-handler, particularly under concurrent load.Benchmarks (in several runs, it's close to 1.20x and 1.30x)
The E2E tests were run on test handler in aws/aws-sdk-js-v3#8014
New E2E tests will be added when
@smithy/undici-http-handleris published.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.