Skip to content

Commit ac493c2

Browse files
rix0rrrgithub-actions
and
github-actions
authored
fix(npmjs): follower hangs when doing a backfill (#1723)
(Re-roll of #1721 which was accidentally merged; in the previous version I still did a `throw` from a non-`new Promise()` callback, which made the exception uncaught. Solve in this one by more strictly splitting out the Promise conversion to helper functions) For reasons of the NPMJS replica behaving strangely[1], our follower has changed to starting a full backfill of the entire catalog. However, when doing the full backfill we're seeing strange behavior: when doing fetches for a lot of packages in parallel, we notice that the follower stops printing anything and the Lambda just times out after 5 minutes of inactivity. Looking at the logs, 47 requests are sent out in parallel but only 15 are answered, and then nothing more happens. Looking at the code it is a mess of NodeJS event handling, and from what I can tell there is no handling of socket error events that occur in the request (just socket error events that occur in the response). Best theory: since these events are dropped on the floor, the corresponding Promise is never resolved or rejected and the follower will just hang. There also was no request timeout, I added that as well. I refactored the code to be more straightforward: - No recursion to do retries but a `while` loop - Use a deadline-driven timeout (previous code would retry indefinitely) - Put the classification of what makes an error retryable in once place - Introduce helper functions to handle the conversion between callback style and `async/await`. - Importantly: add `req.on('error')` and `req.on('timeout')` to handle problems with the request. - Replace `x = new Error(); Error.captureStackTrace(x); ko(x)` with `throw new Error()`, since we can now do that. [1]: https://github.com/orgs/community/discussions/152515#discussioncomment-13053455 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --------- Signed-off-by: github-actions <github-actions@github.com> Co-authored-by: github-actions <github-actions@github.com>
1 parent 1804404 commit ac493c2

File tree

3 files changed

+70
-54
lines changed

3 files changed

+70
-54
lines changed

src/__tests__/__snapshots__/construct-hub.test.ts.snap

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/__tests__/devapp/__snapshots__/snapshot.test.ts.snap

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/package-sources/npmjs/couch-changes.lambda-shared.ts

Lines changed: 64 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { EventEmitter } from 'events';
2-
import { OutgoingHttpHeaders } from 'http';
2+
import { IncomingMessage, OutgoingHttpHeaders } from 'http';
33
import { Agent, request, RequestOptions } from 'https';
44
import { Readable } from 'stream';
55
import { URL } from 'url';
@@ -155,53 +155,30 @@ export class CouchChanges extends EventEmitter {
155155
let maxDelay = 100;
156156
while (true) {
157157
try {
158-
return await new Promise((ok, ko) => {
159-
const req = request(url, requestOptions, (res) => {
160-
if (res.statusCode == null) {
161-
throw new RetryableError('No status code available');
162-
}
163-
164-
// Server errors. We can't know whether these are really retryable but we usually pretend that they are.
165-
if (res.statusCode >= 500 && res.statusCode < 600) {
166-
throw new RetryableError(
167-
`HTTP ${res.statusCode} ${res.statusMessage}`
168-
);
169-
}
170-
171-
// Permanent (client) errors:
172-
if (res.statusCode >= 400 && res.statusCode < 500) {
173-
throw new Error(`HTTP ${res.statusCode} ${res.statusMessage}`);
174-
}
175-
176-
console.log(
177-
`Response: ${method.toUpperCase()} ${url} => HTTP ${
178-
res.statusCode
179-
} (${res.statusMessage})`
180-
);
181-
182-
res.once('error', ko);
183-
184-
const json = JSONStream.parse(true);
185-
json.once('data', ok);
186-
json.once('error', ko);
187-
188-
const plainPayload =
189-
res.headers['content-encoding'] === 'gzip' ? gunzip(res) : res;
190-
plainPayload.pipe(json, { end: true });
191-
plainPayload.once('error', ko);
192-
});
193-
194-
req.on('error', ko);
195-
req.on('timeout', () => {
196-
req.destroy(
197-
new RetryableError(
198-
`Timeout after ${REQUEST_ATTEMPT_TIMEOUT_MS}ms, aborting request`
199-
)
200-
);
201-
});
202-
203-
req.end(body && JSON.stringify(body, null, 2));
204-
});
158+
const res = await requestPromise(url, requestOptions, body);
159+
if (res.statusCode == null) {
160+
throw new RetryableError('No status code available');
161+
}
162+
163+
// Server errors. We can't know whether these are really retryable but we usually pretend that they are.
164+
if (res.statusCode >= 500 && res.statusCode < 600) {
165+
throw new RetryableError(
166+
`HTTP ${res.statusCode} ${res.statusMessage}`
167+
);
168+
}
169+
170+
// Permanent (client) errors:
171+
if (res.statusCode >= 400 && res.statusCode < 500) {
172+
throw new Error(`HTTP ${res.statusCode} ${res.statusMessage}`);
173+
}
174+
175+
console.log(
176+
`Response: ${method.toUpperCase()} ${url} => HTTP ${
177+
res.statusCode
178+
} (${res.statusMessage})`
179+
);
180+
181+
return await readResponseJson(res);
205182
} catch (e: any) {
206183
if (Date.now() > deadline || !isRetryableError(e)) {
207184
throw e;
@@ -216,6 +193,45 @@ export class CouchChanges extends EventEmitter {
216193
}
217194
}
218195

196+
/**
197+
* A Promisified version of `https.request()` that also handles timeout events
198+
*/
199+
function requestPromise(
200+
url: URL,
201+
options: RequestOptions,
202+
body?: Record<string, unknown>
203+
) {
204+
return new Promise<IncomingMessage>((ok, ko) => {
205+
const req = request(url, options ?? {}, ok);
206+
req.on('error', ko);
207+
req.on('timeout', () => {
208+
req.destroy(
209+
new RetryableError(
210+
`Timeout after ${REQUEST_ATTEMPT_TIMEOUT_MS}ms, aborting request`
211+
)
212+
);
213+
});
214+
req.end(body && JSON.stringify(body, null, 2));
215+
});
216+
}
217+
218+
function readResponseJson(
219+
res: IncomingMessage
220+
): Promise<Record<string, unknown>> {
221+
return new Promise((ok, ko) => {
222+
res.once('error', ko);
223+
224+
const json = JSONStream.parse(true);
225+
json.once('data', ok);
226+
json.once('error', ko);
227+
228+
const plainPayload =
229+
res.headers['content-encoding'] === 'gzip' ? gunzip(res) : res;
230+
plainPayload.pipe(json, { end: true });
231+
plainPayload.once('error', ko);
232+
});
233+
}
234+
219235
class RetryableError extends Error {}
220236

221237
function isRetryableError(e: Error): boolean {

0 commit comments

Comments
 (0)