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(all): ensure a exception catch when async send call to a dead object; #5805

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

saraivagilberto
Copy link
Contributor

@saraivagilberto saraivagilberto commented Mar 11, 2021

Hello devs,

Please accept this fix to mitigate a problem with a scenario of intense concurrency headless use;

Recently I migrate from puppeteer to this project targeting a better performance, and so it is, but in the path I found a async function been called without a await. Resulting, in a high performance application, a case of send to a dead object on the Dispatch(s):

<gracefully close start>
Error
at /home/ubuntu/project_name/node_modules/playwright-firefox/lib/server/firefox/ffConnection.js:147:63
at new Promise ()
at FFSession.send (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/server/firefox/ffConnection.js:146:16)
at FFNetworkManager.setRequestInterception (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/server/firefox/ffNetworkManager.js:39:29)
at FFPage.updateRequestInterception (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/server/firefox/ffPage.js:285:36)
at Page._setClientRequestInterceptor (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/server/page.js:254:30)
at PageDispatcher.setNetworkInterceptionEnabled (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/dispatchers/pageDispatcher.js:125:20)
at DispatcherConnection.dispatch (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/dispatchers/dispatcher.js:149:52)
at Immediate. (/home/ubuntu/project_name/node_modules/playwright-firefox/lib/inprocess.js:45:85)
at processImmediate (node:internal/timers:463:21)
at process.topLevelDomainCallback (node:domain:147:15)
at process.callbackTrampoline (node:internal/async_hooks:130:14)

Current example was on playwright-firefox package, but the fix reflects on all Browser Types.

Thanks, att
Gilberto

@ghost
Copy link

ghost commented Mar 11, 2021

CLA assistant check
All CLA requirements met.

@saraivagilberto saraivagilberto changed the title fix(alll): ensure a exception catch when async send call to a dead object; fix(all): ensure a exception catch when async send call to a dead object; Mar 11, 2021
Copy link
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

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

Do you have a test case that we could add to our test suite?

@yury-s yury-s merged commit 7fcb892 into microsoft:master Mar 12, 2021
@saraivagilberto
Copy link
Contributor Author

It's a really big combination of mismatches on the launch of the browser and the OS resources failure (sometimes memory, or PID limits).

What can I do is point out some triggers that I followed to get to the solution, in the case of this example with Firefox:
https://github.com/microsoft/playwright/blob/master/src/server/firefox/ffConnection.ts#L78

When the connection fails and the SIGINT triggers on the launch resulting in the gracefullyClose event call
https://github.com/microsoft/playwright/blob/master/src/server/processLauncher.ts#L141

@mxschmitt
Copy link
Member

@yury-s I think it might make sense to enable the following ESLint rule to prevent it in the future: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-floating-promises.md or what do you think?

@saraivagilberto
Copy link
Contributor Author

And for some reason I believe the problem occur from a broken async chain call, right above the problematic line, the async call chain is respected;
https://github.com/microsoft/playwright/pull/5805/files#diff-1a279c35116efd1f9673ec4a032aaddaa7ccba32b9db74e1c710b33229a4528eR114

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