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

fix: handle page close during response event #398

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

vigneshshanmugam
Copy link
Member

  • Trying to access some of the API's like headers, sizes would trigger unhandled error as the underlying page would be closed before the response event is finished.

Note: This technique is used by Hartracer in Playwright and we use the same here, we might move to PW HarTracer once its extendable.

@@ -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.

@apmmachine
Copy link

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-13T18:59:50.860+0000

  • Duration: 16 min 23 sec

  • Commit: ddafedb

Test stats 🧪

Test Results
Failed 0
Passed 140
Skipped 0
Total 140

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

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.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM!

@vigneshshanmugam vigneshshanmugam merged commit 5e4826f into elastic:master Oct 14, 2021
@vigneshshanmugam vigneshshanmugam deleted the add-barriers branch October 14, 2021 14:57
v1v added a commit to dominiqueclarke/synthetics that referenced this pull request Nov 22, 2021
…a-fleet-heartbeat-synthetics-e2e-tests

* upstream/master:
  [jjbb] for scheduling e2e jobs (elastic#418)
  docs: release notes for beta.17 (elastic#415)
  1.0.0-beta.17
  feat: add --playwright-options (elastic#405)
  fix: ensure timestamps of events are inorder (elastic#413)
  fix: do not stash `.nvm` folder (elastic#414)
  deprecate suite-params option over params (elastic#406)
  fix: end timestamp should reflect duration (elastic#410)
  docs: release notes for 1.16.0 (elastic#403)
  1.0.0-beta.16
  fix: revert Playwright to 1.14.0 (elastic#402)
  docs: release notes for beta 15 (elastic#400)
  1.0.0-beta.15
  chore: update deps and fix lint issues (elastic#399)
  fix: handle page close during response event (elastic#398)
  chore: require node > 14 for usage (elastic#397)
  fix: fix megabytes to mbps (elastic#394)
  feat: enable trace as part of rich-events (elastic#387)
  feat: use events from PW for network data (elastic#372)
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

Successfully merging this pull request may close these issues.

3 participants