-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor body handling to use ReadableStream #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: conico/core-rewrite
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| // @ts-nocheck | ||
| import http from "node:http"; | ||
| import type { ReadableStream } from "node:stream/web"; | ||
|
|
||
| export class IncomingMessage extends http.IncomingMessage { | ||
| constructor({ | ||
|
|
@@ -16,7 +17,7 @@ export class IncomingMessage extends http.IncomingMessage { | |
| method: string; | ||
| url: string; | ||
| headers: Record<string, string | string[]>; | ||
| body?: Buffer; | ||
| body?: ReadableStream; | ||
| remoteAddress?: string; | ||
| }) { | ||
| super({ | ||
|
|
@@ -28,12 +29,6 @@ export class IncomingMessage extends http.IncomingMessage { | |
| destroy: Function.prototype, | ||
| }); | ||
|
|
||
| // Set the content length when there is a body. | ||
| // See https://httpwg.org/specs/rfc9110.html#field.content-length | ||
| if (body) { | ||
| headers["content-length"] ??= String(Buffer.byteLength(body)); | ||
| } | ||
|
|
||
| Object.assign(this, { | ||
| ip: remoteAddress, | ||
| complete: true, | ||
|
|
@@ -46,9 +41,53 @@ export class IncomingMessage extends http.IncomingMessage { | |
| url, | ||
| }); | ||
|
|
||
| this._read = () => { | ||
| this.push(body); | ||
| this.push(null); | ||
| }; | ||
| this._read = (() => { | ||
| if (!body) { | ||
| return () => { | ||
| this.push(null); | ||
| }; | ||
| } | ||
| const reader = body.getReader(); | ||
| let reading = false; | ||
| let streamDone = false; | ||
|
|
||
| this.once("close", () => { | ||
| if (!streamDone) { | ||
| streamDone = true; | ||
| reader.cancel().catch(() => {}); | ||
| } | ||
|
Comment on lines
+55
to
+58
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it worth waiting for the current read to finish before cancelling, if there's on in-progress? something like toggle streamdone, wait for reading to be false, and in the pump below dont do it if streamdone is true, then cancel here after reading is false? WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we could do something like that. I don't see any downsides to this at the moment |
||
| }); | ||
|
|
||
| const pump = () => { | ||
| reading = true; | ||
| reader | ||
| .read() | ||
| .then(({ done, value }) => { | ||
| if (done) { | ||
| streamDone = true; | ||
| reader.releaseLock(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm shouldnt the release also happen in the catch as well? |
||
| this.push(null); | ||
| } else { | ||
| const canContinue = this.push(value); | ||
| if (canContinue) { | ||
| pump(); | ||
| } else { | ||
| reading = false; | ||
| } | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| streamDone = true; | ||
| reader.cancel().catch(() => {}); | ||
| this.destroy(err); | ||
| }); | ||
| }; | ||
|
|
||
| return () => { | ||
| if (!reading) { | ||
| pump(); | ||
| } | ||
| }; | ||
| })(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if this is going to be unnecessary for the majority of apps. as in, if someone isn't going to read the body in the middleware, we're probably going ot tee the stream but never make use of that. i wonder if there's some like lazy tee or something we could do potentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know anything about a lazy tee, and to be fair in most case there will be no body.
I guess we could add an option (or an env variable) to not forward the body to the middleware if not needed