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

Hangs on revalidated response from cache #2317

Open
2 tasks done
paescuj opened this issue Dec 9, 2023 · 0 comments
Open
2 tasks done

Hangs on revalidated response from cache #2317

paescuj opened this issue Dec 9, 2023 · 0 comments

Comments

@paescuj
Copy link
Contributor

paescuj commented Dec 9, 2023

Describe the bug

  • Node.js version: 20
  • OS & version: macOS 14

got hangs on revalidated cached response. That is, a response gets revalidated because max-age is reached, while the cached body is still valid (e.g. matching etag).

Actually, it seems like there's already a test, currently marked as failing, exactly covering this case:

test.failing('revalidated uncompressed responses are retrieved from cache', withServer, async (t, server, got) => {


I've tracked the issue down to

const rawBody = await getStreamAsBuffer(from);

which is called in
const success = await this._setRawBody();

I think the issue here is that the response (coming from cache, thus no actual response) never finishes, so getStreamAsBuffer waits indefinitely.


At first, I thought a fix could be as simple as

diff --git a/source/core/index.ts b/source/core/index.ts
index 36e5cf8..75b4dad 100644
--- a/source/core/index.ts
+++ b/source/core/index.ts
@@ -834,9 +834,13 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			response.pause();
 		});
 
-		response.once('end', () => {
-			this.push(null);
-		});
+		if (this._isFromCache) {
+			this.push(null);
+		} else {
+			response.once('end', () => {
+				this.push(null);
+			});
+		}
 
 		if (this._noPipe) {
 			const success = await this._setRawBody();

While that allows the promise to be resolved, it caused other test cases to fail.
I'm afraid I'm too unfamiliar with the code base to come up with a suitable solution, but happy to help if pointed in the right direction.

Actual behavior

Hangs indefinitely, respectively crashes the process if used in a top-level await.

Expected behavior

Should return revalidated cached response

Code to reproduce

import got from 'got';

class SimpleCache {
	cached;

	get() {
		return this.cached;
	}

	set(_, value) {
		const entry = JSON.parse(value);
		// To force immediate revalidation
		entry.value.cachePolicy.rescc['max-age'] = '0';

		this.cached = JSON.stringify(entry);
	}
}

const cache = new SimpleCache();

// npm registry is a good showcase
const url = 'https://registry.npmjs.org/got';

let response = await got(url, { cache });
console.log(response.isFromCache);

response = await got(url, { cache });
console.log(response.isFromCache);

Run with:

node test.js; echo $?

Because it's a top-level await, Node.js correctly detects that the promise will never get resolved (exit code 13):

false
13

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
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

No branches or pull requests

1 participant