Skip to content

Commit

Permalink
✨ Discovery retry support + fixed a race condition in network handling (
Browse files Browse the repository at this point in the history 
#1529)

* Added a filter to ignore already finished urls in request race

* v1.28.1-alpha.1

* Added retry support on discovery queue

* Added support to abort retriable task

* v1.28.1-alpha.2

* temp workaround for tests

* v1.28.1-alpha.3

* Added a test case for retry

* Renamed tag to beta

* Added a test for request race in browser
  • Loading branch information
@ninadbstack
ninadbstack committed Mar 8, 2024
1 parent 0a48e87 commit d772b43
Show file tree
Hide file tree
Showing 7 changed files with 200 additions and 34 deletions.
7 changes: 6 additions & 1 deletion packages/core/src/config.js
View file
Expand Up @@ -234,6 +234,10 @@ export const configSchema = {
type: 'integer',
minimum: 1
},
retry: {
type: 'boolean',
default: false
},
launchOptions: {
type: 'object',
additionalProperties: false,
Expand Down Expand Up @@ -281,7 +285,8 @@ export const snapshotSchema = {
captureMockedServiceWorker: { $ref: '/config/discovery#/properties/captureMockedServiceWorker' },
captureSrcset: { $ref: '/config/discovery#/properties/captureSrcset' },
userAgent: { $ref: '/config/discovery#/properties/userAgent' },
devicePixelRatio: { $ref: '/config/discovery#/properties/devicePixelRatio' }
devicePixelRatio: { $ref: '/config/discovery#/properties/devicePixelRatio' },
retry: { $ref: '/config/discovery#/properties/retry' }
}
}
},
Expand Down
71 changes: 41 additions & 30 deletions packages/core/src/discovery.js
View file
Expand Up @@ -8,7 +8,8 @@ import {
createPercyCSSResource,
createLogResource,
yieldAll,
snapshotLogName
snapshotLogName,
withRetries
} from './utils.js';

// Logs verbose debug logs detailing various snapshot options.
Expand Down Expand Up @@ -301,37 +302,47 @@ export function createDiscoveryQueue(percy) {
let assetDiscoveryPageEnableJS = (snapshot.cliEnableJavaScript && !snapshot.domSnapshot) || (snapshot.enableJavaScript ?? !snapshot.domSnapshot);

percy.log.debug(`Asset discovery Browser Page enable JS: ${assetDiscoveryPageEnableJS}`);
// create a new browser page
let page = yield percy.browser.page({
enableJavaScript: assetDiscoveryPageEnableJS,
networkIdleTimeout: snapshot.discovery.networkIdleTimeout,
requestHeaders: snapshot.discovery.requestHeaders,
authorization: snapshot.discovery.authorization,
userAgent: snapshot.discovery.userAgent,
captureMockedServiceWorker: snapshot.discovery.captureMockedServiceWorker,
meta: snapshot.meta,

// enable network inteception
intercept: {
enableJavaScript: snapshot.enableJavaScript,
disableCache: snapshot.discovery.disableCache,
allowedHostnames: snapshot.discovery.allowedHostnames,
disallowedHostnames: snapshot.discovery.disallowedHostnames,
getResource: u => snapshot.resources.get(u) || cache.get(u),
saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } }
}
});

try {
yield* captureSnapshotResources(page, snapshot, {
captureWidths: !snapshot.domSnapshot && percy.deferUploads,
capture: callback,
captureForDevices: percy.deviceDetails || []
await withRetries(async function*() {
// create a new browser page
let page = yield percy.browser.page({
enableJavaScript: assetDiscoveryPageEnableJS,
networkIdleTimeout: snapshot.discovery.networkIdleTimeout,
requestHeaders: snapshot.discovery.requestHeaders,
authorization: snapshot.discovery.authorization,
userAgent: snapshot.discovery.userAgent,
captureMockedServiceWorker: snapshot.discovery.captureMockedServiceWorker,
meta: snapshot.meta,

// enable network inteception
intercept: {
enableJavaScript: snapshot.enableJavaScript,
disableCache: snapshot.discovery.disableCache,
allowedHostnames: snapshot.discovery.allowedHostnames,
disallowedHostnames: snapshot.discovery.disallowedHostnames,
getResource: u => snapshot.resources.get(u) || cache.get(u),
saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } }
}
});
} finally {
// always close the page when done
await page.close();
}

try {
yield* captureSnapshotResources(page, snapshot, {
captureWidths: !snapshot.domSnapshot && percy.deferUploads,
capture: callback,
captureForDevices: percy.deviceDetails || []
});
} finally {
// always close the page when done
await page.close();
}
}, {
count: snapshot.discovery.retry ? 3 : 1,
onRetry: () => {
percy.log.info(`Retrying snapshot: ${snapshotLogName(snapshot.name, snapshot.meta)}`, snapshot.meta);
},
signal: snapshot._ctrl.signal,
throwOn: ['AbortError']
});
})
.handle('error', ({ name, meta }, error) => {
if (error.name === 'AbortError' && queue.readyState < 3) {
Expand Down
11 changes: 10 additions & 1 deletion packages/core/src/network.js
View file
Expand Up @@ -31,6 +31,7 @@ export class Network {
#requests = new Map();
#authentications = new Set();
#aborted = new Set();
#finishedUrls = new Set();

constructor(page, options) {
this.page = page;
Expand Down Expand Up @@ -86,6 +87,13 @@ export class Network {
}

requests = Array.from(this.#requests.values()).filter(filter);
// remove requests which are finished at least once
// this happens when same request is made multiple times by browser in parallel and one of
// them gets stuck in pending state in browser [ need to debug why ]. So we dont receive
// loadingFinished event, causing it to show up in Active requests, but we can only store one
// response per url so as long as we have captured one, we dont care about other such requests
requests = requests.filter((req) => !this.#finishedUrls.has(req.url));

return requests.length === 0;
}, {
timeout: Network.TIMEOUT,
Expand Down Expand Up @@ -131,9 +139,10 @@ export class Network {
}

// Called when a request should be removed from various trackers
_forgetRequest({ requestId, interceptId }, keepPending) {
_forgetRequest({ requestId, interceptId, url }, keepPending) {
this.#requests.delete(requestId);
this.#authentications.delete(interceptId);
this.#finishedUrls.add(url);

if (!keepPending) {
this.#pending.delete(requestId);
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/queue.js
View file
Expand Up @@ -79,11 +79,17 @@ export class Queue {

// call or set up other handlers
let exists = this.cancel(item);
task.ctrl = new AbortController();
// duplicate abortion controller on task, so it can can be used in further
// generators and can be cancelled internally
// TODO fix this for non object item usecase
if (typeof item === 'object' && !Array.isArray(item) && item !== null) {
item._ctrl = task.ctrl;
}
task.item = item = this.#handlers.push
? this.#handlers.push(item, exists) : item;
task.handler = () => this.#handlers.task
? this.#handlers.task(item, ...args) : item;
task.ctrl = new AbortController();

// queue this task & maybe dequeue the next task
this.#queued.add(task);
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/snapshot.js
View file
Expand Up @@ -118,7 +118,8 @@ function getSnapshotOptions(options, { config, meta }) {
disableCache: config.discovery.disableCache,
captureMockedServiceWorker: config.discovery.captureMockedServiceWorker,
captureSrcset: config.discovery.captureSrcset,
userAgent: config.discovery.userAgent
userAgent: config.discovery.userAgent,
retry: config.discovery.retry
}
}, options], (path, prev, next) => {
switch (path.map(k => k.toString()).join('.')) {
Expand Down
19 changes: 19 additions & 0 deletions packages/core/src/utils.js
View file
Expand Up @@ -324,6 +324,25 @@ export function serializeFunction(fn) {
return fnbody;
}

export async function withRetries(fn, { count, onRetry, signal, throwOn }) {
count ||= 1; // default a single try
let run = 0;
while (true) {
run += 1;
try {
return await generatePromise(fn, signal);
} catch (e) {
// if this error should not be retried on, we want to skip errors
let throwError = throwOn?.includes(e.name);
if (!throwError && run < count) {
await onRetry?.();
continue;
}
throw e;
}
}
}

export function snapshotLogName(name, meta) {
if (meta?.snapshot?.testCase) {
return `testCase: ${meta.snapshot.testCase}, ${name}`;
Expand Down
115 changes: 115 additions & 0 deletions packages/core/test/discovery.test.js
View file
Expand Up @@ -910,6 +910,121 @@ describe('Discovery', () => {
'^\\[percy:core:discovery] Setting PERCY_NETWORK_IDLE_WAIT_TIMEOUT over 60000ms is not'
));
});

describe('with multiple network requests with same url', () => {
beforeEach(async () => {
// a custom page where we make 2 requests to same url where only one of them will
// finish before network idle wait timeout, in such cases we expect to ignore another
// request stuck in loading state
server.reply('/', () => [200, 'text/html', dedent`
<html>
<body>
<P>Hello Percy!<p>
<script>
// allow page load to fire and then execute this script
setTimeout(async () => {
var p1 = fetch('./img.gif');
var p2 = fetch('./img.gif');
await p2; // we will resolve second request instantly
await p1; // we will delay first request by 800ms
}, 10);
</script>
</body>
</html>`
]);
// trigger responses in expected time
let counter = 0;
// we have idle timeout at 500 ms so we are resolving other request at 1 sec
server.reply('/img.gif', () => new Promise(r => (
(counter += 1) && setTimeout(r, counter === 2 ? 0 : 1000, [200, 'image/gif', pixel]))));
});

it('shows debug info when navigation fails within the timeout', async () => {
percy.loglevel('debug');

await percy.snapshot({
name: 'navigation idle',
url: 'http://localhost:8000'
});

expect(logger.stderr).not.toContain(jasmine.stringMatching([
'^\\[percy:core] Error: Timed out waiting for network requests to idle.',
'',
' Active requests:',
' - http://localhost:8000/img.gif',
'',
'(?<stack>(.|\n)*)$'
].join('\n')));
});
});
});

describe('discovery retry', () => {
let Page;
let fastCount;

beforeEach(async () => {
// reset page timeout so that it gets read from env again
({ Page } = await import('../src/page.js'));
Page.TIMEOUT = undefined;
process.env.PERCY_PAGE_LOAD_TIMEOUT = 500;

// some async request that takes a while and only resolves 4th time
let counter = 0;
server.reply('/', () => new Promise(r => (
(counter += 1) &&
setTimeout(r, counter === fastCount ? 0 : 2000, [200, 'text/html', '<html></html>']))));
});

afterAll(() => {
delete process.env.PERCY_PAGE_LOAD_TIMEOUT;
});

it('should retry the snapshot discovery upto 3 times', async () => {
// 3rd request will resolve instantly
fastCount = 3;

await percy.snapshot({
name: 'test navigation timeout',
url: 'http://localhost:8000',
discovery: { retry: true },
widths: [400, 800]
});

await percy.idle();

expect(logger.stdout).toEqual([
'[percy] Percy has started!',
'[percy] Retrying snapshot: test navigation timeout',
'[percy] Retrying snapshot: test navigation timeout',
'[percy] Snapshot taken: test navigation timeout'
]);
});

it('throws exception after last retry', async () => {
// 3rd request will also resolve in delayed fashion
fastCount = 4;

await percy.snapshot({
name: 'test navigation timeout',
url: 'http://localhost:8000',
discovery: { retry: true },
widths: [400, 800]
});

await percy.idle();

expect(logger.stdout).toEqual([
'[percy] Percy has started!',
'[percy] Retrying snapshot: test navigation timeout',
'[percy] Retrying snapshot: test navigation timeout'
]);

expect(logger.stderr).toEqual([
'[percy] Encountered an error taking snapshot: test navigation timeout',
'[percy] Error: Navigation failed: Timed out waiting for the page load event'
]);
});
});

describe('navigation timeout', () => {
Expand Down

0 comments on commit d772b43

Please sign in to comment.