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(nsis): Ignore other users processes #6472

Merged

Conversation

I-Otsuki
Copy link
Contributor

@I-Otsuki I-Otsuki commented Dec 4, 2021

Only find and kill processes owned by current user on per-user install.

Closes #6104

Only find and kill processes owned by current user on install.
On per-user installation, other users run their own copy of the app.
These instances will not block install and are should be left untouched.

fixes electron-userland#6104
Change to not relying on taskkill's return value.
taskkill returns 0 when at least one process is killed even if there are
any other process not killed.
@changeset-bot
Copy link

changeset-bot bot commented Dec 4, 2021

🦋 Changeset detected

Latest commit: d602e1c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@indutny-signal
Copy link
Contributor

Just a quick FYI, we are investigating a report from a person on Windows 10 Education being unable to update. So far everything indicates that this commit might be the culprit.

I have a feeling that the Education version of Windows may come without some of the command-line tools, but I'm yet to confirm this.

@I-Otsuki
Copy link
Contributor Author

I-Otsuki commented Apr 6, 2022

@indutny-signal
Thank you for pointing this out.
The code I wrote assumes return code of non-zero as signal of no running instance.
If we have tasklist or find missing, then cmd returns non-zero value so the installer just fails to detect running app...!
In that case, it will try to continue and then fail installing files...
I'll take a look at the report.

@indutny-signal
Copy link
Contributor

@I-Otsuki I think the code is correct and I honestly updated dozens of times myself on both Windows 10 and Windows 11 before we shipped it to users. We have a number of reports from people running into this and so far it looks like it is a small percentage of users, but it is far from a singular event as far as I can tell.

Unfortunately, we had another unrelated issue with yellow banner so things are bit confusing in the bug tracker now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NSIS install fails on terminal servers with multiple users already running the program
3 participants