Skip to content
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

A ReadableStream branch was created but never consumed #648

Closed
HaidongPang opened this issue Nov 6, 2024 · 7 comments · Fixed by #661
Closed

A ReadableStream branch was created but never consumed #648

HaidongPang opened this issue Nov 6, 2024 · 7 comments · Fixed by #661

Comments

@HaidongPang
Copy link

I'm using ky in cloudflare worker with following code:

import ky from "ky";
export default {
    async fetch (): Promise<Response> {
        const result = await ky.get("https://www.github.com").text();
        return new Response(result);
    }
} satisfies ExportedHandler;

and when I turn off local mode, requests raise the following warning:

A ReadableStream branch was created but never consumed. Such branches can be created, for instance, by calling the tee() method on a ReadableStream, or by calling the clone() method on a Request or Response object. If a branch is created but never consumed, it can force the runtime to buffer the entire body of the stream in memory, which may cause the Worker to exceed its memory limit and be terminated. To avoid this, ensure that all branches created are consumed.

 * Unused stream created:
    at result.<computed> [as text] (file:///....../node_modules/ky/source/core/Ky.ts:92:36)
......

Locate the following code snippet:

source/core/Ky.ts

export class Ky {
	static create(input: Input, options: Options): ResponsePromise {
		const ky = new Ky(input, options);

		const function_ = async (): Promise<Response> => {
                         ......
		};

		......

		for (const [type, mimeType] of Object.entries(responseTypes) as ObjectEntries<typeof responseTypes>) {
			result[type] = async () => {
				......
				const awaitedResult = await result;
				const response = awaitedResult.clone();

				if (type === 'json') {
					if (response.status === 204) {
						return '';
					}

					const arrayBuffer = await response.clone().arrayBuffer();
					const responseSize = arrayBuffer.byteLength;
					if (responseSize === 0) {
						return '';
					}

					if (options.parseJson) {
						return options.parseJson(await response.text());
					}
				}

				return response[type]();
			};
		}

		return result;
	}

The awaitedResult is a variable of type Response. Since it is declared, it has been cloned only once, and its ReadableStream has never been consumed.
After awaitedResult has finished cloning, its ReadableStream should be closed to ensure memory safety.
Seems like:

const awaitedResult = await result;
const response = awaitedResult.clone();
await awaitedResult.body.cancel();
@sholladay
Copy link
Collaborator

PR welcome to fix this issue. If I'm not mistaken, canceling the stream could lead to lost data even though we've cloned the stream. We'd need to test it thoroughly to make sure that's not the case.

HaidongPang added a commit to HaidongPang/ky that referenced this issue Nov 7, 2024
Clean up orphan ReadableStream remaining during parsing response (sindresorhus#648)
@HaidongPang
Copy link
Author

I‘m willing to do so, and a PR have already been submitted .
Regarding your concerns, here is a simple verification.

export default {
    async fetch (): Promise<Response> {
        const originalResponse = new Response("Some Data");
        const clonedResponse = originalResponse.clone();
        await originalResponse.body?.cancel();
        console.log("Original Stream Data:", await originalResponse.body?.getReader().read());
        console.log("Cloned Stream Data:", await clonedResponse.body?.getReader().read());
        return new Response("");
    }
} satisfies ExportedHandler;

I cloned an unconsumed response and canceled the ReadableStream of the original response before consuming the cloned response.

The result is as follows:

Original Stream Data: Object {
  done: true
}
Cloned Stream Data: Object {
  value: Uint8Array(9),
  done: false
}

The original ReadableStream has already been canceled, but the cloned response has an independent ReadableStream object that can be consumed.

@HaidongPang
Copy link
Author

There is also a similar issue about orphan ReadableStream remaining.

Related to :#650

@TheHolyWaffle
Copy link

I'm not sure if this applies to Cloudflare Workers, but the undici team released version 6.19.8 which will automatically garbage collect response streams if the response object is out of scope.

See nodejs/undici#2856

sholladay added a commit to sholladay/ky that referenced this issue Dec 9, 2024
Fixes sindresorhus#648

This removes an unnecessary response clone that was used in body method shortcuts, such as `ky().json()`, which caused the original response to be fully buffered and never consumed.
@work-in-progress-danny
Copy link

#661 might not have fixed this. I'm on v1.7.4 and getting this error

const response = await ky.post("...url...", {
  throwHttpErrors: false,
  retry: { limit: 0 },
  headers: {
    "Content-Type": "application/json",
    Authorization: `Bearer ${ctx.env.API_KEY}`,
  },
  json: <...data...>,
})

if (!response.ok) {
  console.error(response.statusText)
  return
}

const jsonResponse = await response.json()
ReadableStream branch was created but never consumed. Such branches can be created, for instance, by calling the tee() method on a ReadableStream, or by calling the clone() method on a Request or Response object. 
If a branch is created but never consumed, it can force the runtime to buffer the entire body of the stream in memory, which may cause the Worker to exceed its memory limit and be terminated.
To avoid this, ensure that all branches created are consumed.

 * Unused stream created:
    at Ky._fetch (file://..../node_modules/ky/source/core/Ky.ts:305:30)
    at function_ (file://..../node_modules/ky/source/core/Ky.ts:38:28)
    at async <function> (file://..../function.ts:30:20)

@sholladay
Copy link
Collaborator

@work-in-progress-danny you are probably encountering #650, which is technically a separate but similar problem that leads to the same outcome. Think of it as a "Part 2", if you will.

There is already a PR to fix it, see #651. I was hoping to release these together but something has been causing CI to fail on that PR and I'm not sure why yet. It passes locally for me. The failure might be related to usage of httpbin.org in the tests, which would make it unrelated to that PR, but that's just a guess. Once that is solved, we'll get that PR merged and released quickly. Feel free to help investigate the CI failure if you have time.

@work-in-progress-danny
Copy link

Awesome, thanks so much @sholladay! Love this package 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants