feat(core): add config to skip waiting for browser close#120
feat(core): add config to skip waiting for browser close#120doteric wants to merge 1 commit intofacebook:mainfrom
Conversation
|
Hi @doteric! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
doteric
force-pushed
the
feat/no_wait_browser_close
branch
2 times, most recently
from
283fa8a to
3d49eb4
Compare
doteric
force-pushed
the
feat/no_wait_browser_close
branch
from
3d49eb4 to
1e17020
Compare
| await Promise.all(pages.map(page => page.close())); | ||
| await browser.disconnect(); | ||
| } else if (config.skipBrowserCloseWait === true) { | ||
| browser.close(); |
There was a problem hiding this comment.
In this case, skipping waiting for the promising to resolve may allow memlab to proceed, but after memory leak detection is done, the JS runtime would still not able to terminate since it would wait for all promises to resolve. Do you force terminate the node.js runtime somehow in the end?
There was a problem hiding this comment.
Thanks for the question!
We use Memlab inside AWS Lambdas where we do not have to wait for the runtime to finish. It keeps running and in the meantime if the browser finally closes then good, if not then AWS Lambda will terminate the runtime anyway. Ideally, of course, would be the browser.close() to not hang and just close the browser properly, but it's not so straightforward why the closing hangs, it would require deeper investigation.
In some cases iterating through all the opened pages and closing them before closing the browser worked, but in case of Memlab this does not seem to work correctly as the browser instance seems to return no opened pages.
Sparticuz/chromium#85 (comment)
Please let me know if I explained this fine or would you want further clarification. Happy to discuss further if needed
There was a problem hiding this comment.
@doteric That makes sense. Thanks for the context. Please check and accept the CLA agreement so I can import and integrate your change.
https://github.com/facebook/memlab/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@JacksonGL has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@JacksonGL merged this pull request in 3024612. |
3 participants
Issue: #95