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

Restart does not close old Electron instance #251

Open
probablykasper opened this issue Sep 12, 2024 · 23 comments · Fixed by #236 · May be fixed by #253
Open

Restart does not close old Electron instance #251

probablykasper opened this issue Sep 12, 2024 · 23 comments · Fixed by #236 · May be fixed by #253
Labels
good first issue Good for newcomers

Comments

@probablykasper
Copy link

Problem

My app has a before-quit handler which waits for the webview to be ready. But when the plugin makes Electron restart, the webview page/code is gone, so it can't tell the app to finish quitting. This means I end up with many Electron instances running at the same time

Solution

Keep the Vite webpage loaded until the app quits

Workaround

Disable restarting:

onstart({ startup }) {
  if (process.electronApp) {
    console.log('\x1b[35mNot restarting\x1b[0m')
  } else {
    startup()
  }
}
@yejimeiming
Copy link
Contributor

This problem happened before, and we solved it using treeKillSync.
Do you still have similar problems on you local machine?

treeKillSync(process.electronApp.pid!)

export function treeKillSync(pid: number) {
if (process.platform === 'win32') {
cp.execSync(`taskkill /pid ${pid} /T /F`)
} else {
killTree(pidTree({ pid, ppid: process.pid }))
}
}

@probablykasper
Copy link
Author

Yes I do have the problem on the latest version (v0.28.7).

However, it works if I call treeKillSync in my own Vite config, or even just process.kill(process.electronApp.pid):

onstart({ startup }) {
	if (process.electronApp) {
		treeKillSync(process.electronApp.pid)
		// or
		process.kill(process.electronApp.pid)
	}
	startup()
},

It would be neat if it did a graceful shutdown with SIGTERM by waiting for the child_process to exit

@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 18, 2024

@probablykasper Would you like to test the following changes locally?

process.kill(tree.pid) // #214

function killTree(tree: PidTree) {
  if (tree.children) {
    for (const child of tree.children) {
      killTree(child)
    }
  }

  try {
-   process.kill(tree.pid) // #214
+   process.kill(tree.pid, 'SIGKILL') // Force kill process
  } catch { /* empty */ }
}

caoxiemeihao added a commit that referenced this issue Sep 18, 2024
@caoxiemeihao caoxiemeihao linked a pull request Sep 18, 2024 that will close this issue
@probablykasper
Copy link
Author

That does work. Personally I would prefer waiting for the child process to exit though instead of force quitting

@probablykasper
Copy link
Author

Here's a working solution without force quitting:

startup.exit = async () => {
  if (process.electronApp) {
    process.electronApp.removeAllListeners();
    process.kill(process.electronApp.pid, 'SIGTERM');
    await new Promise((resolve) => {
      process.electronApp.once("exit", resolve);
    })
  }
};

@probablykasper
Copy link
Author

Yes, it works for me. But I don't know how you want to incorporate treeKillSync

@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 19, 2024

Maybe you could try this for me, it should work fine.
treeKillSync is required, we need to take care of people using Windows systems.

startup.exit = async () => {
  if (process.electronApp) {
    await new Promise((resolve) => {
      process.electronApp.removeAllListeners()
      process.electronApp.once('exit', resolve)
      treeKillSync(process.electronApp.pid!)
    })
  }
}

caoxiemeihao added a commit that referenced this issue Sep 19, 2024
@yejimeiming

This comment was marked as outdated.

@probablykasper
Copy link
Author

That does not work unfortunately. It doesn't quit, and gives me this error:

[47765:0919/044144.460406:ERROR:gpu_process_host.cc(993)] GPU process exited unexpectedly: exit_code=15
[47765:0919/044144.468629:ERROR:network_service_instance_impl.cc(601)] Network service crashed, restarting service.

@yejimeiming

This comment was marked as outdated.

@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 19, 2024

image

The same error happened to me, but vite-plugin-electron still works fine. This error is thrown by Electron, it's not caused by us.

@probablykasper
Copy link
Author

I am on macOS, yes. Are you testing with a before-quit handler like I use though?
https://github.com/probablykasper/ferrum/blob/7ac23dd2ca8fa397e51355aab6e0283f2e11be5a/src/electron/main.ts#L151-L168

@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 19, 2024

I am on macOS, yes. Are you testing with a before-quit handler like I use though? https://github.com/probablykasper/ferrum/blob/7ac23dd2ca8fa397e51355aab6e0283f2e11be5a/src/electron/main.ts#L151-L168

This seems to be very tricky. treeKillSync(pid) directly kills the Renderer process(child process of Main process), causing the ipc handle you registered in before-quit to fail.
If we use process.kill(pid) instead of treeKillSync(pid), this will cause some Windows users to be unable to quit the previous App process instance.


The current known way is to circumvent this problem by customizing the startup.exit().

import electron, { startup } from 'vite-plugin-electron'

startup.exit = async () => {
  if (process.electronApp) {
    process.electronApp.removeAllListeners()
    process.kill(process.electronApp.pid)
  }
}

@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 19, 2024

@probablykasper You can try this. This may seem like a complete hack. But it can at least help us circumvent the problem we are facing now.

import { resolve } from 'path'
import { defineConfig } from 'vite'
import { svelte, vitePreprocess } from '@sveltejs/vite-plugin-svelte'
import electron, { startup } from 'vite-plugin-electron'
import tailwindcss from '@tailwindcss/vite'

+ startup.exit = async () => {
+   if (process.electronApp) {
+     process.electronApp.removeAllListeners()
+     process.kill(process.electronApp.pid)
+   }
+ }

export default defineConfig({
  base: '/',
  clearScreen: false,
  resolve: {
    alias: {
      '@': resolve(__dirname, './src'),
    },
  },
  build: {
    outDir: './build/web',
    sourcemap: true,
    target: 'chrome106',
  },
  plugins: [
    svelte({
      preprocess: vitePreprocess(),
    }),
    tailwindcss(),
    electron({
      entry: ['./src/electron/main.ts', './src/electron/preload.ts'],
-     onstart({ startup }) {
-       if (process.electronApp) {
-         process.kill(process.electronApp.pid, 'SIGTERM')
-       } else {
-         startup()
-       }
-     },
      vite: {
        build: {
          outDir: './build/electron',
          emptyOutDir: true,
          rollupOptions: {
            external: [/^.*\.node$/],
          },
        },
      },
    }),
  ],
})

@probablykasper
Copy link
Author

If we use process.kill(pid) instead of treeKillSync(pid), this will cause some Windows users to be unable to quit the previous App process instance.

Do Wondows users have that issue even with the .once('exit', resolve)? Could be worth testing.

Seems weird if the tree-kill is the only way to quit the app 🤔

@yejimeiming yejimeiming reopened this Sep 20, 2024
@yejimeiming

This comment was marked as duplicate.

caoxiemeihao added a commit that referenced this issue Sep 20, 2024
@caoxiemeihao caoxiemeihao linked a pull request Sep 20, 2024 that will close this issue
@yejimeiming
Copy link
Contributor

yejimeiming commented Sep 20, 2024

Do Wondows users have that issue even with the .once('exit', resolve)? Could be worth testing.

Seems weird if the tree-kill is the only way to quit the app 🤔

I created a v0.28.9 branch which may fix this issue.
Would you like to build the v0.28.9 branch locally and test if it works?

I tried your example and it works fine on v0.28.9 branch.

@probablykasper
Copy link
Author

It does not work unfortunately. I made a reproduction repo here: https://github.com/probablykasper/vite-plugin-electron-bug

@yejimeiming
Copy link
Contributor

It does not work unfortunately. I made a reproduction repo here: https://github.com/probablykasper/vite-plugin-electron-bug

Private repo?

@probablykasper
Copy link
Author

So sorry, public now

@kokororin
Copy link

kokororin commented Sep 21, 2024

I write a module to terminate Electron zombie processes, which has been successfully used in my project for a long time. It has only been tested on Windows.

import electron from 'vite-plugin-electron/simple';
import { killElectron } from 'kill-electron';

export default defineConfig({
  plugins: [
    electron({
      onstart: args => {
        if (os.platform() === 'win32') {
          spawn('chcp', ['65001']);
        }
      
        if (process.env.VSCODE_DEBUG) {
          console.log(
            /* For `.vscode/.debug.script.mjs` */ '[startup] Electron App'
          );
        } else {
          void args.startup();
        }
        // Orphaned Electron processes can occur during debugging
        // To prevent system slowdowns, these processes need to be killed before each startup
        killElectron({
          zombieOnly: true
        });
      },
// ... rest code of vite config

@onburn
Copy link

onburn commented Sep 29, 2024

vite-plugin-electron v0.28.8
vite v5.3.1

In mac, when I make modifications in the main process main.js, the BrowserWindow instance of electron will be re-created,
What should I do?

export default defineConfig({
  plugins: [
    vue(),
    vueJsx(),
    vueDevTools(),
    electron([
      {
        entry: "electron/main.js",
      },
      {
        entry: "electron/preload.js",
      },
    ]),
    electronRenderer(),
  ],
})

@hifron
Copy link

hifron commented Oct 14, 2024

also present when stopped and are more instances which uses the same dev port...

Some of this https://github.com/vite-plugin repos may solve multiple instances or dev instances, but when there are multiple clients and users of Electron apps on Desktop, then this issue become something more when ports could be assigned for same port in different frameworks or different plugins or something different than Electron...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
5 participants