fether icon indicating copy to clipboard operation
fether copied to clipboard

Exiting still requires CTRL+C

Open ltfschoen opened this issue 6 years ago • 4 comments

  • When run with yarn electron and then once it has loaded exit by right clicking the icon in the Dock and choosing Quit, in the terminal it fully exits and shows the prompt (i.e. ~/code/src/paritytech/fether - [master] $ ). But when run with "yarn start" instead of exiting the same way it doesn't return to the prompt, it just hangs and displays the following and it's necessary to press CTRL+C to kill it
[start-electron] ┏ Electron -------------------
[start-electron] 
[start-electron]   [2019-01-08T02:38:46.817Z] INFO (@parity/electron:main/62633 on lss-MacBook-Pro.local): Another instance of parity is already running on http://127.0.0.1:8546, skip running local instance.
[start-electron] 
[start-electron] ┗ ----------------------------
[start-react   ] Compiled successfully!
[start-react   ] 
[start-react   ] You can now view fether-react in the browser.
[start-react   ] 
[start-react   ]   Local:            http://localhost:3000/
[start-react   ]   On Your Network:  http://192.168.43.40:3000/
[start-react   ] 
[start-react   ] Note that the development build is not optimized.
[start-react   ] To create a production build, use npm run build.
[start-react   ] 
[start-react   ] Compiling...
[start-react   ] Compiled successfully!
  • When I run with yarn electron and then once it has loaded exit by going to Electron menu File > Quit, in the terminal it fully exits and shows the prompt (i.e. ~/code/src/paritytech/fether - [master] $ ). But when run with "yarn start" instead of exiting the same way it doesn't return to the prompt, it just hangs and displays similar to previous message and it's necessary to press CTRL+C to kill it

  • When run with yarn electron or yarn start and then once it has loaded exit by pressing CTRL+C in terminal it fully exits and shows the prompt (i.e. ~/code/src/paritytech/fether - [master] $ ).

Potential solutions:

  • Option 1 - Remove all event listeners before quiting. Check if there are still event listeners hanging and remove them https://github.com/electron/electron/issues/9862#issuecomment-311074928

i.e. something like

    app.on('something', () => {
      const listenerNames = ['did-this', 'did-that'];

      listenerNames.forEach((name, index) => {
        app.removeAllListeners(name);
      });

      pino.info('Remaining listeners: ', 
        events.EventEmitter.listenerCount(app, 'event'));
    });
  • Option 2 - pkill library will makes it fully quit, but doesn't exit gracefully: Try by adding dependency "pkill": "^2.0.0", And updating:
app.on('quit', () => {
  killParity();
  pkill.full('paritytech/fether', function (err, validPid) {
    console.log('err', err);
    console.log('validPid', validPid); // if matched any processes
  });
  pino.info('Exited Fether with PKill so no need to press CTRL+C to return to terminal prompt');
});
  • Option 3: node-powershell library or custom script...
  • Option 4: No action, assume that the user will just press CTRL+C if we show a pino log like 'Exited Fether. Press CTRL+C'

ltfschoen avatar Jan 08 '19 02:01 ltfschoen

Do you know it this happens with the packaged version (.dmg, .exe, .deb)? if not then it's not a big deal.

Tbaut avatar Jan 08 '19 11:01 Tbaut

Using .dmg Fether v0.1.2 on macOS Mojave 10.14. I tested it by running ps aux | grep "Fether" after launching Fether to view its running processes. Then if I exit by either exit by right clicking the icon in the Dock and choosing Quit or exit by going to Electron menu File > Quit I found that when I ran ps aux | grep "Fether" again afterwards the process was no longer listed.

ltfschoen avatar Jan 08 '19 12:01 ltfschoen

Is there are script we could use to automatically check this happens for new versions that we're about to publish?

ltfschoen avatar Jan 08 '19 12:01 ltfschoen

We really don't need to make this work with yarn start. yarn start spawns multiple processes:

  • one for Electron
  • one for create-react-app
  • one for building fether-ui

What you're suggesting is that when we kill the process that runs Electron, the 2 others are also killed. I don't think it's needed, and quite hard to achieve. yarn start is only for dev, and killing with CTRL+C is more than enough.

Concerning the binary, we need to make sure that the process is killed when we quit the app. If you can create an e2e test for that, it'll be great. If not, we'll just manually test it, like we do right now.

amaury1093 avatar Jan 08 '19 13:01 amaury1093