node-pty
node-pty copied to clipboard
microsoft
Proposal: Rewrite of native API using rust and N-API
Issue description
I am looking into rewriting the existing implementation of this native library in rust with N-API. I am working on a branch currently where I will publish the changes I am proposing, and I would love feedback on it once I publish the branch as WIP.
Reasons for the rewrite
There are multiple issues regarding cross platform support and Node version that would all be resolved in a rewrite in rust. It would also make solving issues such as the blocking semantics mentioned in #85 much easier.
How does the community feel about this?
If I do write this and make sure there are no errors, will this be used?
I am a fan of a full rewrite of this lib, esp. to fix those nasty low level blocking bugs, and to expose somewhat more control to the outside (termios interface). Idk if Rust is a good choice for this, is that easy enough to integrate for CI stuff and such?
On a sidenote: I already fixed #85 for POSIX systems way back (in C++), but it was not merged, because it contained too many internal chances.
Rust will definitely still integrate well with CI from my understanding. I will take a look at #85 as well to make sure that those changes would be integrated as a part of this.
From the rewrite that i've done thus far with no changes to how the API functions, the code is already quite a bit smaller and easier to understand.
I am hoping that this change will be welcomed by the community and help to advance this library moving forward.
Can't say for sure without seeing it, but Rust would probably be way better from my perspective and would force me to learn it more which I've wanted to do for a while anyway.
After looking at #85 my first response would be that I am going to aim to do this work with the current working version on master, and then i'll add enhancements from #85 after if it is still decided that we would like that API added in. For the moment it is just easier since there is already a decent bit of code I am rewriting.
@daniel-brenot I hope you can find a better way around the POLLHUP issue on linux, than my noisy poll loop. Should be possible with a dedicated polling thread injecting the data into the JS runtime. If you wonder why I re-routed the data packages via OS pipes - that was the only way I found working with theSocket interface on JS side. Maybe thats easier now with N-API primitives, idk.
@jerch I think once the initial migration is done we can definitely look at what's available and make an issue to do this.
Sure thing. Another issue I want to point out before you get your hands dirty into the wrong direction - #487 and its issues behind. It deals with a recent fork slowdown under BigSur, but got not yet merged. So be prepared for some forth and back changes on your side as well.
Created a branch with the new changes. So far it's mostly just the one file and it's still incomplete, but it is taking shape. https://github.com/daniel-brenot/node-pty/blob/rust-port/src/lib.rs
With rust, you can implement Node.js library based on existed Rust crates, like https://crates.io/crates/portable-pty
@Brooooooklyn Great suggestion! I've looked at that library and it doesn't seem to provide enough functionality vs what we currently have to justify it in this use case.
Most of the code in the rewrite is due to the fork() function. The rest is relatively trivial.
Thank you for the suggestion though :)
@Tyriar How would you feel if the process method threw an error instead of returning undefined? It would align with the rest of the methods that throw errors when they fail. It's currently implemented as returning undefined in my version, but i'm wondering if that should be changed to allow for an error message when the process call fails.
If you mean with "process method" the pty & process creation step - thats actually another open issue (see #265). Ideally the lib exposes enough error state to the caller to know why things went wrong.
#46 Can also factor into this. Precompiling for multiple platforms will be a part of this as well
Hi 👋 I've taken the code from @daniel-brenot and got it to work using napi-rs 2.0.0.alpha4. Currently open and fork work and some of the tests even pass. There are issues with encoding and signals.
Only Unix for now... I have very little experience with Windows.
Here are the (preliminary) results of my effort. https://github.com/corwin-of-amber/node-pty/tree/rust-port
Are ppl interested in this?
That's great! I'm happy to see that you've gotten it working on unix. I definitely still intend to work on this, but work has been demanding as of late.
If it's complete, absolutely. That being said, definitely ping me once you're done, i'd love to help on the work you're doing as well moving forward where i can
The Unix port is complete. Why don't I make a PR for your fork, then you can look at it and add Windows support when you have the time.
Because of the amount of changes a PR to move to rust would include, I'm thinking we can set up a branch with CI/CD so people can start selfhosting ASAP. That will also make merging easier as the it can be stabilized on that branch.
FYI a big refactor happened in https://github.com/microsoft/node-pty/pull/487
Also, it might be best to maintain a fork for now and consider merging into node-pty when it's more stable (or continue on as a hard fork). I'd hate to see progress stalled because of me being the bottleneck to merge things.
Bumping this as a heads up, I am still working on this. I was on a bit of a hiatus, but i am currently working on adapting the program to use the modified build process for NAPI and doing some more touches on ensuring that everything is safe. Thanks a ton to @corwin-of-amber for his contributions to my branch, they helped a ton.
As an update, i think i'm going to update the CI for my branch to run and validate the code against the tests so i can see if my branch is passing. Additionally, i would like to look at bundling in the .node files for multiple platforms as a part of the release process. It would mean that users wouldn't need to compile for their platform if a .node file is found. It's quite simple to implement so i'm trying is out on my branch to see how well it works.
Does anyone know the purpose of exposing the native module? I noticed this is done only on linux platforms and i'm curious as to why. Is this used by VSCode or other libraries? I need to know because some of the changes I made will break the exposed native module if I don't account for it, so i need to know wether I strictly need to maintain the current behaviour, or if a small change to it is acceptable.
Edit: Heres the file i'm refering to: https://github.com/microsoft/node-pty/blob/main/src/index.ts
@corwin-of-amber Should I add your name to the copyright notice as well for the files you assisted with? If so, should I add the name that is on your github profile? (Shachar Itzhaky)
I definitely like having my name on things 😄 as long as it does not block others from using this code as free software.
Great! Also if you could have a look at the code on my branch it is fairly close to being complete, but with something preventing it from connecting to the named pipes on windows. I may take a look at it some time this week to see if i can find out why, but if you feel like having a look and seeing if you can find it then i'd appreciate annother set of eyes :)