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

Fail to install update because of app process running (Windows) #8026

Open
rossicler-hostalky opened this issue Feb 1, 2024 · 13 comments
Open

Comments

@rossicler-hostalky
Copy link

  • Electron-Builder Version: ^23.6.0
  • Electron-Updater Version: ^5.3.0
  • Node Version: 16.x
  • Electron Version: ^23.0.0
  • Electron Type (current, beta, nightly):
  • Target: Windows (nsis)

I'm having an issue with WIndows when trying to install the update.

The problem is that after quitting the app, there's a process "hanging" that is not closed after quitting the app, which gives the user an error when trying to update the app.

I know that this issue is not on electron-updater side, but I'm wondering if there's anything I can do to force electron-updater to close any running process of my application, because this process that doesn't quit is a bug under investigation in another library, and it doesn't seem like they will fix it quickly.

Examples:
When I run .quitAndInstall(), the UI will show to the user that the application is running and he needs to close, and it won't work unless the user go to task manager and close the running process.

autoUpdater.on('update-downloaded', (info) => {
  sendStatusToWindow(ChannelsTypes.UPDATE_DOWNLOADED, info.version);
  setTimeout(() => autoUpdater.quitAndInstall(), 500);
});

I also tried adding isSilent and isForceRunAfter to true, but what happens is that the app closes to install in silent mode, but it doesn't open again (probably because it failed to install), but since it's on silent mode, it won't show any errors to the user.

autoUpdater.on('update-downloaded', (info) => {
  sendStatusToWindow(ChannelsTypes.UPDATE_DOWNLOADED, info.version);
  setTimeout(() => autoUpdater.quitAndInstall(true, true), 500);
});
@mmaietta
Copy link
Collaborator

mmaietta commented Feb 2, 2024

I think this may be fixed in the next electron-updater package via #7955

@rossicler-hostalky
Copy link
Author

@mmaietta Do you have an idea when this PR will be included in a release?
I checked next version (v6.1.8) and it doesn't seem to include this PR, is that correct?

@mmaietta
Copy link
Collaborator

mmaietta commented Feb 5, 2024

My apologies, I didn't realize that PR was in app-builder-lib for some reason. The PR changes are in next electron-builder, more specifically v24.11.0

@rossicler-hostalky
Copy link
Author

@mmaietta thanks the quick reply. I tried again with electron-build to v24.11.0 and electron-updater v6.1.8 and still didn't work.

FYI, I can see that the process hanging has a long name, which according to #7955 might be a problem, but since this one supposedly fixed the issue, I'm not sure if the problem is something else. To be more exact, the process hanging name has 28 characters.

Tomorrow I will try renaming my application to a smaller name and try again, and reply back here with my findings.
Thanks again for the support.

@rossicler-hostalky
Copy link
Author

rossicler-hostalky commented Feb 7, 2024

@mmaietta I just tested and even with a small process name (8 characters), it stills fails to kill the process and install the update.
Any idea what else I can do?

EDIT:
I also tried running without silent mode, but same result.
image

EDIT 2:
I'm digging a little bit more on this, and by checking the file allowOnlyOneInstallerInstance.nsh, I see that this script runs tasklist to get the all process from the application, and taskill to terminate them (if needed). I tried "simulating" this in a windows machine, and by running cmd /c tasklist /FI "IMAGENAME eq <my_app>", I can see the process that needs to be killed, but when I run cmd /c taskkill /f /im <my_app>, it says:

ERROR: The process with PID <number> could not be terminated.
Reason: There is no running instance of the task.

That is probably the issue that this script is encountering and why it can't kill the process.

I tested and I can successfully kill the process by running wmic process where name="<my_app>" call terminate.
Would this be a valid option to replace taskkill used? If so, I'm happy to create a PR for it.

@rossicler-hostalky
Copy link
Author

I have applied the "workaround" mentioned above using patch-package and now it's all working in my end.
In summary I updated the file allowOnlyOneInstallerInstance.nsh by replacing all taskkill commands to wmic process where name="<my_app>" call terminate. The only "issue" about this is that it doesn't have some filters that taskkill had, like /fi "PID ne $pid or /fi "USERNAME eq %USERNAME%".

@rossicler-hostalky
Copy link
Author

@mmaietta any input on this?

@mmaietta
Copy link
Collaborator

mmaietta commented Mar 6, 2024

Hmmm, we need the username filter to make sure we don't kill all apps by other users logged in on the system, right? This would be applicable for user-isolated installations (INSTALL_MODE_PER_ALL_USERS === null)

I'm not too familiar with nsis scripting or with wmic to make a truly informed input here though

@rossicler-hostalky
Copy link
Author

I'm not very familiar with it either, and I forgot most things I found when I did this. But as far as I remember, there are ways to filter by username, but it's not a one liner like taskkill.

Feel free to close this issue if you don't think this will be a problem for other users. If I have some time in a few days, I can try to get back at this and add the filter by username, then I could create a PR.

@Ahbrown41
Copy link

This is not a usable solution, creates issues when auto upgrading the application as it creates a race condition between old and new instances.

@rossicler-hostalky
Copy link
Author

This is not a usable solution, creates issues when auto upgrading the application as it creates a race condition between old and new instances.

What do you mean by "creates a race condition"? Both commands terminate the old instances before applying the update. The only issue I can see is what was discussed above.

@Ahbrown41
Copy link

So, if I utilize the task kill command, when I have a lot of devices out in the field, it looks as though, as the device is transitioning between the old version and the new version that there are occasions when this command is killing both the old and the new versions and then no version is running. Appears to be worse on slower machines.

My guess is this has something to do with the transition between the old version and the new version.

@rossicler-hostalky
Copy link
Author

Ahbrown41

I see, so it still might be a problem with the filters, since it's killing based on the name, and not PID, it might kill both if they are running at the same time. If my assumption is correct, it's still all tied to the filters missing, which is possible to implement, but in my solution, I didn't.

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

No branches or pull requests

3 participants