fixed issue #559

[shamun-khatri]

Linked Issue(s)

Closes #559

Proposed changes (including videos or screenshots)

I have used process.exit() in one place (useEffect hook where it checks if AppState === TERMINATED) instead of exit() that is not working. I have tried to solve this issue by debugging and trying other methods but noting works but this.

Here is a screenshot of working terminal detach.

Further comments

I hope this will get merged. But if you think I could do better then tell me I will change things along.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ shamun-khatri
❌ Shamun Khatri

---

Shamun Khatri seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[jayantbh]

I have a feeling that this doesn't always work.
For example, if you let the ./dev.sh script run till completion (or long enough), and then press X, it doesn't quit successfully.

I noticed you replaced ink's exit call with process.exit, which is fine, but the dependencies remain unchanged.

I think I'd have kept both the process.exit and exit calls in there.

Additionally...
There's a lot of whitespace changes which I'm not sure are intentional.

[shamun-khatri]

Yes, whitespaces are not intentional I have tried many other things that's why they are there.

But I got confused about what will not work. You said in a long-running process that might now work why?

Also, what does it mean that dependencies remain unchanged?

[shamun-khatri]

One thing I forgot about I have tried keeping both exit() and process.exit() and keeping both also works.

And I have also tried keeping the app(process) running for 5 minutes and then tried X with only process.exit() and it still works fine.

[jayantbh]

But I got confused about what will not work.
Honestly, same. 😆

I wasn't able to reliably reproduce that issue in the past.

But yeah, I think call exit and then process.exit. Call both. exit from ink allows it to do any needed cleanup before actual exit.

And do remove any unintended code/whitespace changes. I think that'll be it for this PR.

[thesujai]

I hovered around inks codebase and found the reason exit() isn't working is some processes are still active. So the exit() by useApp isn't detaching the terminal.
The processref.current.kill() call in the handleExit is not working as intended.

So this solution introduced here might be a hacky way to do things, but it doesn't break anything. Just we can do a check that after calling exit(), if the process is still not killed, then call the process.exit(0) (to keep in mind this isn't forced as code 0 is to safely kill the process). This will ensure that no process is running after the process is unmounted. Else failure to do so might result in terminal being unmounted, but some services still running in the bg.

[jayantbh]

Interesting. Yeah I can see how that happens.
How do we check if a process hasn't safely and completely exited?

[thesujai]

After exit() call in useEffect(), can do:

if(processRef.current) {
process.exit(0);
}

[shamun-khatri]

@thesujai I have tried your solution and it is working fine. But why some process is still running there after running Ikn's exit().

And are you raising a PR for this, cause you did better job. Or should I raise a PR?

[thesujai]

But why some process is still running there after running Ikn's exit()

Try running git blame on this file and see what introduced this bug(Maybe it worked before, not sure)

are you raising a PR for this

No, and feel free to update this branch. Good job!

[shamun-khatri]

@jayantbh @thesujai, I have made changes as per the discussion.

I have tried to go a little deep-down on this and find that processRef.current.kill(); is returning false this means the process it is containing is not killing. I have also tried processRef.current.kill('SIGKILL') which should force to kill the process but this also gives false. It should give true so that it successfully kills the process.

I took reference from here https://nodejs.org/api/child_process.html#subprocesskillsignal

So I think the solution we discussed to use both the exits is fine because I think we are using docker and in the end, it is turning off successfully.

[jayantbh]

Why the processRef check here?
Also, this doesn't look formatted right.

[shamun-khatri]

After exit() call in useEffect(), can do:

if(processRef.current) {
process.exit(0);
}

@jayantbh As suggested in this comment I think it would be better to check for any process before calling process.exit(0) .
And I will look at formatting next time (Will format for this changes). but should I check for the running process before calling process.exit(0) or not?

[jayantbh]

I understand the intent, but given that you didn't really do anything with processRef itself, and that exit is being called anyway, I think it's okay to just kill it.

A check for processRef would fit better if we were doing something with it directly or indirectly.

Right now if processRef happens to not be set, then the UI is potentially just not going to exit properly, which isn't desired behaviour either.

[jayantbh]

Btw, you could join our OSS Community on Slack for quick back and forth and discussions.
https://mhq.link/oss-community

[shamun-khatri]

Ok, I will just kill that and remove unnecessary checks.I have joined the community on slack thanks for sharing.

[shamun-khatri]

I made changes. Please have a look.

[jayantbh]

@shamun-khatri, please use better commit names to better reflect the changes within.

I wrote about this a while ago.
https://dev.to/middleware/not-heres-how-to-write-actually-good-commit-messages-hint-its-not-just-adding-commit-lint-j2i

[shamun-khatri]

@jayantbh I have renamed the commit. I tried my best to write that but if you thing I am still missing anything then do let me know. Also, could you assign me before merging? If it's not counting in Hacktoberfest