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.

Sign up for GitHub

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

Devlow: refactor browser.ts #6286

Merged
merged 6 commits into from
Nov 6, 2023
Merged

Devlow: refactor browser.ts #6286

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2eebbb
Devlow: refactor browser.ts
wbinnssmith Oct 26, 2023
aeffe02
fixup! Devlow: refactor browser.ts
wbinnssmith Oct 26, 2023
dfe18af
fixup! Devlow: refactor browser.ts
wbinnssmith Oct 26, 2023
433d2b1
Merge remote-tracking branch 'origin/main' into wbinnssmith/devlow
wbinnssmith Oct 27, 2023
65891dc
WIP
sokra Nov 6, 2023
19a51d3
await async calls
sokra Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/devlow-bench/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"inquirer": "^9.2.7",
"minimist": "^1.2.8",
"pidusage-tree": "^2.0.5",
"playwright-chromium": "^1.35.0",
"playwright-chromium": "^1.39.0",
"split2": "^4.2.0",
"tree-kill": "^1.2.2"
}
Expand Down
118 changes: 80 additions & 38 deletions packages/devlow-bench/src/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ interface BrowserSession {
reload(metricName: string): Promise<void>;
}

const browserOutput = !!process.env.BROWSER_OUTPUT;
const browserOutput = Boolean(process.env.BROWSER_OUTPUT);

async function withRequestMetrics(
metricName: string,
page: Page,
fn: () => Promise<void>
): Promise<void> {
const activePromises: Promise<void>[] = [];
const activePromises: Array<Promise<void>> = [];
const sizeByExtension = new Map<string, number>();
const requestsByExtension = new Map<string, number>();
const responseHandler = (response: Response) => {
Expand All @@ -32,14 +32,17 @@ async function withRequestMetrics(
const url = response.request().url();
const status = response.status();
const extension =
/^[^\?#]+\.([a-z0-9]+)(?:[\?#]|$)/i.exec(url)?.[1] ?? "none";
// eslint-disable-next-line prefer-named-capture-group -- TODO: address lint
/^[^?#]+\.([a-z0-9]+)(?:[?#]|$)/i.exec(url)?.[1] ?? "none";
const currentRequests = requestsByExtension.get(extension) ?? 0;
requestsByExtension.set(extension, currentRequests + 1);
if (status >= 200 && status < 300) {
let body;
try {
body = await response.body();
} catch {}
} catch {
// empty
}
if (body) {
const size = body.length;
const current = sizeByExtension.get(extension) ?? 0;
Expand Down Expand Up @@ -145,12 +148,19 @@ async function withRequestMetrics(
}
}

/**
* Waits until network requests have all been resolved
* @param page - Playwright page object
* @param delayMs - Amount of time in ms to wait after the last request resolves before cleaning up
* @param timeoutMs - Amount of time to wait before continuing. In case of timeout, this function resolves
* @returns
*/
function networkIdle(
page: Page,
delay: number = 300,
rejectTimeout: number = 180000
) {
return new Promise<void>((resolve, reject) => {
delayMs = 300,
timeoutMs = 180000
): Promise<number> {
return new Promise((resolve) => {
const cleanup = () => {
page.off("request", requestHandler);
page.off("requestfailed", requestFinishedHandler);
Expand All @@ -160,50 +170,71 @@ function networkIdle(
clearTimeout(timeout);
}
};
let activeRequests = 0;

const requests = new Map<string, number>();
const start = Date.now();
let lastRequest: number;
let timeout: NodeJS.Timeout | null = null;
const requests = new Set();

const fullTimeout = setTimeout(() => {
cleanup();
reject(
new Error(
`Timeout while waiting for network idle. These requests are still pending: ${Array.from(
requests
).join(", ")}}`
)
// eslint-disable-next-line no-console -- logging
console.error(
`Timeout while waiting for network idle. These requests are still pending: ${Array.from(
requests
).join(", ")}} time is ${lastRequest - start}`
);
}, rejectTimeout);
const requestFilter = async (request: Request) => {
return (await request.headers().accept) !== "text/event-stream";
resolve(Date.now() - lastRequest);
}, timeoutMs);

const requestFilter = (request: Request) => {
return request.headers().accept !== "text/event-stream";
};
const requestHandler = async (request: Request) => {
requests.add(request.url());
activeRequests++;

const requestHandler = (request: Request) => {
requests.set(request.url(), (requests.get(request.url()) ?? 0) + 1);
if (timeout) {
clearTimeout(timeout);
timeout = null;
}
// Avoid tracking some requests, but we only know this after awaiting
// so we need to do this weird stunt to ensure that
if (!(await requestFilter(request))) {
await requestFinishedInternal(request);
if (!requestFilter(request)) {
requestFinishedInternal(request);
}
};
const requestFinishedHandler = async (request: Request) => {
if (await requestFilter(request)) {

const requestFinishedHandler = (request: Request) => {
if (requestFilter(request)) {
requestFinishedInternal(request);
}
};
const requestFinishedInternal = async (request: Request) => {
requests.delete(request.url());
activeRequests--;
if (activeRequests === 0) {

const requestFinishedInternal = (request: Request) => {
lastRequest = Date.now();
const currentCount = requests.get(request.url());
if (currentCount === undefined) {
// eslint-disable-next-line no-console -- basic logging
console.error(
`Unexpected untracked but completed request ${request.url()}`
);
return;
}

if (currentCount === 1) {
requests.delete(request.url());
} else {
requests.set(request.url(), currentCount - 1);
}

if (requests.size === 0) {
timeout = setTimeout(() => {
cleanup();
resolve();
}, delay);
resolve(Date.now() - lastRequest);
}, delayMs);
}
};

page.on("request", requestHandler);
page.on("requestfailed", requestFinishedHandler);
page.on("requestfinished", requestFinishedHandler);
Expand All @@ -229,8 +260,11 @@ class BrowserSessionImpl implements BrowserSession {
}

async hardNavigation(metricName: string, url: string) {
const page = (this.page = this.page ?? (await this.context.newPage()));
this.page = this.page ?? (await this.context.newPage());

const page = this.page;
await withRequestMetrics(metricName, page, async () => {
/* eslint-disable @typescript-eslint/no-floating-promises -- don't wait for reporting */
measureTime(`${metricName}/start`);
const idle = networkIdle(page, 3000);
await page.goto(url, {
Expand All @@ -247,11 +281,12 @@ class BrowserSessionImpl implements BrowserSession {
measureTime(`${metricName}/load`, {
relativeTo: `${metricName}/start`,
});
await idle;
const offset = await idle;
measureTime(`${metricName}`, {
offset: 3000,
offset,
relativeTo: `${metricName}/start`,
});
/* eslint-enable @typescript-eslint/no-floating-promises -- don't wait for reporting */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this highlights a bug. All measureTime calls should be awaited.

});
return page;
}
Expand All @@ -264,10 +299,13 @@ class BrowserSessionImpl implements BrowserSession {
);
}
await withRequestMetrics(metricName, page, async () => {
/* eslint-disable @typescript-eslint/no-floating-promises -- don't wait for reporting */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

measureTime(`${metricName}/start`);
const firstResponse = new Promise<void>((resolve) =>
page.once("response", () => resolve())
);
const firstResponse = new Promise<void>((resolve) => {
page.once("response", () => {
resolve();
});
});
const idle = networkIdle(page, 3000);
await page.click(selector);
await firstResponse;
Expand All @@ -279,6 +317,7 @@ class BrowserSessionImpl implements BrowserSession {
offset: 3000,
relativeTo: `${metricName}/start`,
});
/* eslint-enable @typescript-eslint/no-floating-promises -- don't wait for reporting */
});
}

Expand All @@ -288,6 +327,7 @@ class BrowserSessionImpl implements BrowserSession {
throw new Error("reload() must be called after hardNavigation()");
}
await withRequestMetrics(metricName, page, async () => {
/* eslint-disable @typescript-eslint/no-floating-promises -- don't wait for reporting */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

measureTime(`${metricName}/start`);
const idle = networkIdle(page, 3000);
await page.reload({
Expand All @@ -309,6 +349,7 @@ class BrowserSessionImpl implements BrowserSession {
offset: 3000,
relativeTo: `${metricName}/start`,
});
/* eslint-enable @typescript-eslint/no-floating-promises -- don't wait for reporting */
});
}
}
Expand All @@ -320,6 +361,7 @@ export async function newBrowserSession(options: {
}): Promise<BrowserSession> {
const browser = await chromium.launch({
headless: options.headless ?? process.env.HEADLESS !== "false",
devtools: true,
timeout: 60000,
});
const context = await browser.newContext({
Expand Down
Loading