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

fix: handle page close during response event #398

Merged
merged 1 commit into from
Oct 14, 2021
+75 −40
Merged

fix: handle page close during response event #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 __tests__/plugins/network.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('network', () => {
const driver = await Gatherer.setupDriver({ wsEndpoint });
const network = new NetworkManager(driver);
await network.start();
await driver.page.goto(server.TEST_PAGE, { waitUntil: 'networkidle' });
await driver.page.goto(server.TEST_PAGE);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the actual test, without this change we will see unhandled error as page object would be closed before the response event fires.

const netinfo = await network.stop();
await Gatherer.stop();
expect(netinfo[0]).toMatchObject({
Expand Down
6 changes: 3 additions & 3 deletions src/common_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ export type Params = Record<string, any>;
export type NetworkConditions = {
offline: boolean;
downloadThroughput: number;
uploadThroughput: number;
latency: number;
uploadThroughput: number;
latency: number;
};
export type HooksArgs = {
env: string;
Expand Down Expand Up @@ -118,7 +118,7 @@ export type Response = {
url?: string;
statusCode: number;
statusText?: string;
mimeType: string;
mimeType?: string;
httpVersion?: string;
headers: Record<string, string>;
// Total size in bytes of the response (body and headers)
Expand Down
107 changes: 71 additions & 36 deletions src/plugins/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
*
*/

import { Request, Response } from 'playwright-chromium';
import { Page, Request, Response } from 'playwright-chromium';
import { NetworkInfo, BrowserInfo, Driver } from '../common_types';
import { Step } from '../dsl';
import { getTimestamp } from '../helpers';
Expand All @@ -49,11 +49,25 @@ type RequestWithEntry = Request & {

export class NetworkManager {
private _browser: BrowserInfo;
private _barrierPromises = new Set<Promise<void>>();
results: Array<NetworkInfo> = [];
_currentStep: Partial<Step> = null;

constructor(private driver: Driver) {}

private _addBarrier(page: Page, promise: Promise<void>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we trying to ensure that we collect header information before the page closes, or just prevent the unhandled exceptions in this case, or both?

Reading through the logic, it's clear to me how this would prevent unhandled exceptions from the page closing before the main promise promise is fulfilled. I'm having more trouble understanding the logic for preventing the page from closing before the data is collected, if that's part of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any async information that we are extracting from the response object or request object should happen during the page lifecycle,

if the page is closed, the associated information is lost and this barrier enforces that we capture the information on the network entry only during the existence of the page.

const race = Promise.race([
new Promise<void>(resolve =>
page.on('close', () => {
this._barrierPromises.delete(race);
resolve();
})
),
promise,
]) as Promise<void>;
this._barrierPromises.add(race);
}

async start() {
const { client, context } = this.driver;
const { product } = await client.send('Browser.getVersion');
Expand Down Expand Up @@ -96,7 +110,6 @@ export class NetworkManager {
},
response: {
statusCode: -1,
mimeType: 'x-unknown',
headers: {},
redirectURL: '',
},
Expand All @@ -122,53 +135,55 @@ export class NetworkManager {
const networkEntry = this._findNetworkEntry(request);
if (!networkEntry) return;

const server = await response.serverAddr();
const responseHeaders = await response.allHeaders();
const mimeType = responseHeaders['content-type']
? responseHeaders['content-type'].split(';')[0]
: 'unknown';

const requestHeaders = await request.allHeaders();

networkEntry.request.headers = requestHeaders;
networkEntry.request.referrer = requestHeaders?.referer;
networkEntry.status = response.status();
networkEntry.responseReceivedTime = epochTimeInSeconds();
networkEntry.response = {
url: response.url(),
statusCode: response.status(),
statusText: response.statusText(),
headers: responseHeaders,
mimeType,
headers: {},
redirectURL: networkEntry.response.redirectURL,
securityDetails: await response.securityDetails(),
remoteIPAddress: server?.ipAddress,
remotePort: server?.port,
};
networkEntry.status = response.status();
networkEntry.responseReceivedTime = epochTimeInSeconds();

const page = request.frame().page();
this._addBarrier(
page,
request.allHeaders().then(reqHeaders => {
networkEntry.request.headers = reqHeaders;
networkEntry.request.referrer = reqHeaders?.referer;
})
);
this._addBarrier(
page,
response.serverAddr().then(server => {
networkEntry.response.remoteIPAddress = server?.ipAddress;
networkEntry.response.remotePort = server?.port;
})
);
this._addBarrier(
page,
response.securityDetails().then(details => {
networkEntry.response.securityDetails = details;
})
);
this._addBarrier(
page,
response.allHeaders().then(resHeaders => {
networkEntry.response.headers = resHeaders;

const mimeType = resHeaders['content-type']
? resHeaders['content-type'].split(';')[0]
: 'unknown';
networkEntry.response.mimeType = mimeType;
})
);
}

private async _onRequestCompleted(request: Request) {
const networkEntry = this._findNetworkEntry(request);
if (!networkEntry) return;

networkEntry.loadEndTime = epochTimeInSeconds();

// For aborted/failed requests sizes does not exist
const sizes = await request.sizes().catch(() => {});
if (sizes) {
networkEntry.request.bytes =
sizes.requestHeadersSize + sizes.requestBodySize;
networkEntry.request.body = {
bytes: sizes.requestBodySize,
};
networkEntry.response.bytes =
sizes.responseHeadersSize + sizes.responseBodySize;
networkEntry.response.body = {
bytes: sizes.responseBodySize,
};
networkEntry.transferSize = sizes.responseBodySize;
}

const timing = request.timing();
const { loadEndTime, requestSentTime } = networkEntry;
networkEntry.timings = {
Expand Down Expand Up @@ -243,6 +258,25 @@ export class NetworkManager {
receive,
total: roundMilliSecs(total),
};

const page = request.frame().page();
// For aborted/failed requests sizes does not exist
this._addBarrier(
page,
request.sizes().then(sizes => {
networkEntry.request.bytes =
sizes.requestHeadersSize + sizes.requestBodySize;
networkEntry.request.body = {
bytes: sizes.requestBodySize,
};
networkEntry.response.bytes =
sizes.responseHeadersSize + sizes.responseBodySize;
networkEntry.response.body = {
bytes: sizes.responseBodySize,
};
networkEntry.transferSize = sizes.responseBodySize;
})
);
}

stop() {
Expand All @@ -251,6 +285,7 @@ export class NetworkManager {
context.on('response', this._onResponse.bind(this));
context.on('requestfinished', this._onRequestCompleted.bind(this));
context.on('requestfailed', this._onRequestCompleted.bind(this));
this._barrierPromises.clear();
return this.results;
}
}