Use pipe_wrap for IO

[lhecker]

I felt like it's easier to test and discuss this when it's on GitHub. So, here we go:
Hacked pipe_wrap for node-pty! I also tuned the setTimeout implementation a bit to use Buffers with the fs.write offset parameter. This way we don't need to slice the string. (If my reading of fs.write is correct and it does return a byte offset, this would also fix a bug in the current code where it uses slice() on the raw string = codepoint offset.)

Closes #833

[deepak1556]

Original version did use pipe_wrap 3913479, however the comment about deprecated api made the transition to the bug prone tty.ReadStream usage for write.

It should be fine now with the fallback we have in place and also the deprecated comment is not correct https://github.com/nodejs/node/blob/05f8772096f974190b11eabce0ea657bc5c8c1b1/lib/internal/bootstrap/realm.js#L84-L163

[deepak1556]

On the other hand, we could implement the pipe_wrap equivalent version using uv_write which does the polling for us and also remove the fallback.

[lhecker]

Original version did use pipe_wrap 3913479, however the comment about deprecated api made the transition to the bug prone tty.ReadStream usage for write.

I knew about pty.js but didn't realize you also used to use this approach!

On the other hand, we could implement the pipe_wrap equivalent version using uv_write which does the polling for us and also remove the fallback.

I have also considered doing this, and possibly go even a step further and not use libuv at all (synchronous PTY calls should be a little bit faster). But @Tyriar suggested that ideally, we'd use as little C/C++ as possible in this project. Reading through node.js didn't give me the impression that process.binding or pipe_wrap are expected to go away any time soon. Perhaps I misjudged that, however. Essentially, we'd simply copy the pipe_wrap code, wouldn't we? I could do that.

[deepak1556]

Reading through node.js didn't give me the impression that process.binding or pipe_wrap are expected to go away any time soon. Perhaps I misjudged that, however

You are right, there was some confusion around this. But my latest take based on nodejs/TSC#1482 (comment) and the redirection to internal binding https://github.com/nodejs/node/blob/05f8772096f974190b11eabce0ea657bc5c8c1b1/lib/internal/bootstrap/realm.js#L84-L163 this is not going away anytime soon.

I have also considered doing this, and possibly go even a step further and not use libuv at all (synchronous PTY calls should be a little bit faster)

It is also the approach currently being implemented in libuv/libuv#4802. The reason we started non-blocking mode is for the PTY host in VS Code to remain responsive to multiple terminal sessions and other process interactions (extension host, workbench), seems like a deal breaker ?

Essentially, we'd simply copy the pipe_wrap code, wouldn't we? I could do that.

Right, I was thinking we replace the write part with native uv_write and the read part can be based off tty.ReadStream. However if we bring in the buffer handling pieces it would almost be equivalent to write implementation in pipe_wrap. Maybe we just go back to only having the current pipe_wrap code in this PR (remove the fallback) till either

1. process.binding('pipe_wrap') gets deprecated, in which case we can fork and implement our own version
2. this modules becomes obsolete in favor of the builtin pty implementation from libuv

@Tyriar thoughts ?

[Tyriar]

If my reading of fs.write is correct and it does return a byte offset, this would also fix a bug in the current code where it uses slice() on the raw string = codepoint offset.

This was a bug with the implementation I was aware of, I just wasn't hitting it in my ascii test as the codepoint offset is the byte offset in that case. #835 looks good but I suspect we still need to handle EAGAIN. Will look at that.

Maybe we just go back to only having the current pipe_wrap code in this PR (remove the fallback) till either

@deepak1556 this would be a hack that you'd have to maintain as monkey patching could break at any time and you're the one that daals with the node updates. Given what we've learned the last week, it's up to you how you'd like to handle it considering:

• The more C++ the harder it is for us to maintain/spot bugs by eyeballing/etc. It's a lot more feasible now though with LLMs to help though.
• Monkey patching is terrible and could break at any time. Forking isn't a bad option, but doesn't that mean 200 loc from https://github.com/nodejs/node/blob/main/src/pipe_wrap.cc plus some from deps too? I don't have faith in our ability to maintain that properly.
• If it makes sense to upstream changes to node we could try that too?

[deepak1556]

If my reading of fs.write is correct and it does return a byte offset, this would also fix a bug in the current code where it uses slice() on the raw string = codepoint offset.

Can you separate this fix into a separate PR, it should be fixed irrelevant to the direction of this PR ?

[lhecker]

Can you separate this fix into a separate PR, it should be fixed irrelevant to the direction of this PR ?

Sure! Although, if we decide to fork pipe_wrap, I suspect that we don't need to do that at all anymore.

[Tyriar]

Turns out we don't need to do this to be super fast: #837