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: support async child process methods without callback in asar #15927

Merged
merged 2 commits into from Dec 18, 2018

Conversation

Projects
None yet
5 participants
@alexgreenland
Copy link
Contributor

commented Dec 3, 2018

Description of Change

In Node land, async child process methods do not require a callback to be provided. In Electron land in the packaged asar environment, there's a bug which means if you don't provide a callback the method does not run — it runs synchronously and blocks execution when a long running child process is launched. I saw this with execFile, I had to add a callback to make the child process run.

In development, original Node is used, but in production, Electron's Node (with changes) is used. This means there is a discrepancy between development and production environments and will catch users out, as it did to me. As it is a silent failure, it's quite difficult to spot.

This change no longer branches to call the synchronous method but continues to use the async method as requested and makes this flow work. I added a new asar test and all asar tests pass.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests added
  • PR title follows semantic commit guidelines

Release Notes

Notes: support async child process methods without callback in asar

@alexgreenland alexgreenland requested a review from as a code owner Dec 3, 2018

@welcome

This comment has been minimized.

Copy link

commented Dec 3, 2018

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@nornagon
Copy link
Contributor

left a comment

This seems reasonable to me! It looks like most of this code was written by @kevinsawicki who's no longer maintaining Electron. Cc @alexeykuzmin for a second review.

@zcbenz
Copy link
Member

left a comment

overrideAPI is also called for fs.open and fs.copyFile, which require the callback to be passed.

As for child_process.execFile, errors should be silently discarded when no callback is passed, currently overrideAPI would try to invoke undefined when path does not exist.

@MarshallOfSound
Copy link
Member

left a comment

In agreement with @zcbenz this will break the invalid usage of async APIs. Perhaps a canBeSync flag passed to overrideAPI might be the solution here

@alexgreenland

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Looking closer, I don't think this has ever worked, the flow from async to sync (overrideAPI -> overrideAPISync).

If you look at the original code:

electron/lib/common/asar.js

Lines 165 to 176 in 90d1c0b

const overrideAPI = function (module, name, pathArgumentIndex) {
if (pathArgumentIndex == null) pathArgumentIndex = 0
const old = module[name]
module[name] = function () {
const pathArgument = arguments[pathArgumentIndex]
const { isAsar, asarPath, filePath } = splitPath(pathArgument)
if (!isAsar) return old.apply(this, arguments)
const callback = arguments[arguments.length - 1]
if (typeof callback !== 'function') {
return overrideAPISync(module, name, pathArgumentIndex)
}

When there's no callback overrideAPI invokes overrideAPISync but overrideAPISync merely overrides the API with module[name] = function (), it crucially doesn't run, which it needs to because we're already in our own invocation module[name] = function () from overrideAPI. It also has the side effect of overriding the async method with the sync method for future invocations.

electron/lib/common/asar.js

Lines 146 to 149 in 90d1c0b

const overrideAPISync = function (module, name, pathArgumentIndex) {
if (pathArgumentIndex == null) pathArgumentIndex = 0
const old = module[name]
module[name] = function () {

I've introduced a more comprehensive change that passes a flag fromAsync from overrideAPI into overrideAPISync when there's no callback. When we detect that we're coming from an async flow, we return the overridden function and overrideAPI invokes it with the original arguments.

My change also means that we don't force the sync method on all further async calls, so if you didn't include a callback when calling once, it will allow you to use a callback in future invocations — there's an at-call check so it doesn't set module[name] in overrideAPISync in this flow.

The new code:

electron/lib/common/asar.js

Lines 146 to 180 in c58517f

const overrideAPISync = function (module, name, pathArgumentIndex, fromAsync) {
if (pathArgumentIndex == null) pathArgumentIndex = 0
const old = module[name]
const func = function () {
const pathArgument = arguments[pathArgumentIndex]
const { isAsar, asarPath, filePath } = splitPath(pathArgument)
if (!isAsar) return old.apply(this, arguments)
const archive = getOrCreateArchive(asarPath)
if (!archive) throw createError(AsarError.INVALID_ARCHIVE, { asarPath })
const newPath = archive.copyFileOut(filePath)
if (!newPath) throw createError(AsarError.NOT_FOUND, { asarPath, filePath })
arguments[pathArgumentIndex] = newPath
return old.apply(this, arguments)
}
if (fromAsync) {
return func
}
module[name] = func
}
const overrideAPI = function (module, name, pathArgumentIndex) {
if (pathArgumentIndex == null) pathArgumentIndex = 0
const old = module[name]
module[name] = function () {
const pathArgument = arguments[pathArgumentIndex]
const { isAsar, asarPath, filePath } = splitPath(pathArgument)
if (!isAsar) return old.apply(this, arguments)
const callback = arguments[arguments.length - 1]
if (typeof callback !== 'function') {
return overrideAPISync(module, name, pathArgumentIndex, true).apply(this, arguments)
}

All original and introduced asar tests pass.

@codebytere
Copy link
Member

left a comment

LGTM with latest changes!

@alexgreenland

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@zcbenz

zcbenz approved these changes Dec 18, 2018

Copy link
Member

left a comment

Nice change!

@zcbenz zcbenz merged commit dc93d94 into electron:master Dec 18, 2018

21 of 23 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
ci/circleci: osx-testing-tests Your tests failed on CircleCI
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
electron-lint Build #20181204.16 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Dec 18, 2018

Release Notes Persisted

support async child process methods without callback in asar

@welcome

This comment has been minimized.

Copy link

commented Dec 18, 2018

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.