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

Can't pty.kill() or pty.destroy() on Windows 10 #34

Closed
matheuss opened this issue Jan 21, 2017 · 13 comments
Closed

Can't pty.kill() or pty.destroy() on Windows 10 #34

matheuss opened this issue Jan 21, 2017 · 13 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@matheuss
Copy link

matheuss commented Jan 21, 2017

Whenever I try to kill a pty process with pty.kill() or pty.destroy(), the whole parent process just crashes:

screen shot 2017-01-21 at 7 12 19 pm

The same happens inside Hyper:

screen shot 2017-01-21 at 7 12 19 pm

I wasn't able to get any relevant logs anywhere, not even on Event Viewer.

A try-catch is useless:

try {
  pty.kill();
} catch (err) {
  console.error(err);
}

The error never gets printed – the parent process just crashes.

If I pty._close(), everything works fine. I cannot reproduce this on macOS or Linux.

Any ideas? 😥

@Tyriar
Copy link
Member

Tyriar commented Jan 21, 2017

I'll have a look, winpty agent probably isn't being cleaned up properly.

@matheuss
Copy link
Author

@Tyriar thank you ❤️ Do you think it's safe to ship Hyper with pty._close() on Windows?

@Tyriar
Copy link
Member

Tyriar commented Jan 21, 2017

I'd avoid shipping just yet as I'll look into this today/tomorrow. FYI I also published 0.6.1 which corrects the compiled JS to fix the Git Bash backspace issue so you can test with that in the meantime. https://github.com/Tyriar/node-pty/releases/tag/0.6.1

@matheuss
Copy link
Author

Same issue on 0.6.1 😕 Will push pty._close() as a temporary workaround then. Thank you again!

@Tyriar
Copy link
Member

Tyriar commented Jan 21, 2017

Yep I can repro and have a test, working on fix now.

    it("should not crash parent process", function(done) {
      const term = new WindowsTerminal('cmd.exe', [], {});
      term.kill();
      // Add done call to deferred function queue to ensure the kill call has completed
      term._defer(done);
    });

@Tyriar Tyriar closed this as completed in 3b154d7 Jan 21, 2017
@Tyriar Tyriar self-assigned this Jan 21, 2017
@Tyriar Tyriar added the bug Issue identified by VS Code Team member as probable bug label Jan 21, 2017
@Tyriar Tyriar added this to the 0.6.2 milestone Jan 21, 2017
@matheuss
Copy link
Author

Amazing @Tyriar ❤️ Any ETA for the release?

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2017

I'll probably release it later today, need to do some verification first.

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2017

@matheuss actually it would be ideal if you could also verify using [email protected] 😄

@matheuss
Copy link
Author

@Tyriar Perfect 💯
screen shot 2017-01-21 at 10 25 08 pm

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2017

@matheuss
Copy link
Author

@Tyriar you're amazing ❤️

@LabhanshAgrawal
Copy link

@Tyriar the typings mention

/**
* Kills the pty.
* @param signal The signal to use, defaults to SIGHUP. This parameter is not supported on
* Windows.
* @throws Will throw when signal is used on Windows.
*/
kill(signal?: string): void;

Is the type doc incorrect?

@LabhanshAgrawal
Copy link

Oops, sorry I misunderstood the @throws stuff, please ignore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

3 participants