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

Porting the Windows conpty part [help needed] #3

Merged
merged 7 commits into from
Mar 31, 2022

Conversation

corwin-of-amber
Copy link

I am trying to port the conpty functionality to Rust as well. I have managed to get it more or less right, but I'm a bit stuck due to my being unfamiliar with Windows API etc.

Currently, I can run a subprocess and get its output, e.g.

const pty = require('.');
var a = pty.spawn("ipconfig.exe", []);
a.on('data', console.log);

And I even get the callback when the process exits. But Node.js does not exit after cleanup... so there must be some resource still hanging there. Can you try this and perhaps help my find what it is?

BTW the code is messy and needs cleanup, of course. But I've got to solve this issue, otherwise this branch is just borked.

Closes microsoft#143.
In accordance with the discussion in the issue thread.
A lot of code still has to be ported and is commented out in `conpty.cc`.
Esp., process exit is not tracked and the corresponding callbacks are not triggered.
Using calls from the winapi create.
This is still incomplete.
@daniel-brenot daniel-brenot merged commit 0a43d2b into daniel-brenot:rust-port Mar 31, 2022
@daniel-brenot
Copy link
Owner

I accepted the MR and am working on the issue in the rust branch. I'm switching out calls to the windows api that you imported for the official windows api for rust. It generally has more idomatic apis and has definitions for things like handles and hpcon.

Some of the issues with node not closing may have to do with some of the fixes that have recently been added. I'm going to continue with trying to get this to work and then see why it is that node isn't exiting on windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants