Expose proper PID on windows and clean up process handle #49
Conversation
There was a problem hiding this comment.
I think your changes make sense.
I noticed that the code was adding winpty_t* pointers to the global ptyHandles vector, but the pointers are never removed. I think that can cause a crash, because remove_pipe_handle will invalidate the winpty pointer by calling winpty_free, but then the pointer is still in the vector. To fix it, you'd want to add something like this to remove_pipe_handle:
ptyHandles.erase(ptyHandles.begin() + i)
You could also remove the winpty_s struct (and the winpty_s::winpty_s() constructor). That code isn't being used at all, and I think it's harmless, but it's a bit of a distraction. There's a real winpty_s inside the winpty DLL, and it has a different definition.
src/win/pty.cc
Outdated
| { | ||
| return Nan::ThrowError("Usage: pty.kill(pid)"); | ||
| } | ||
|
|
||
| int handle = info[0]->Int32Value(); | ||
| HANDLE innerPidHandle = (HANDLE)info[0]->Int32Value(); |
There was a problem hiding this comment.
Is this supposed to be info[1]->Int32Value?
src/win/pty.cc
Outdated
| || !info[0]->IsNumber()) // pid | ||
| if (info.Length() != 2 | ||
| || !info[0]->IsNumber() // pid | ||
| || !info[1]->IsNumber()) // innerPidHandle | ||
| { | ||
| return Nan::ThrowError("Usage: pty.kill(pid)"); |
There was a problem hiding this comment.
nit: I suppose this usage string is now incorrect -- i.e. it should be something like pty.kill(pid, innerPidHandle)
|
@rprichard great feedback thanks, I made the changes and they seem to compile and work fine. |
Fixes #45
@rprichard a quick review of the handle clean up would be greatly appreciated 😃