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

quickfix to get full output #86

Closed
wants to merge 7 commits into from
Closed

Conversation

jerch
Copy link
Collaborator

@jerch jerch commented Jun 3, 2017

Addresses #85 with a quickfix. Do not merge (needs a real solution).

@jerch
Copy link
Collaborator Author

jerch commented Jun 3, 2017

Second commit contains a full working solution, still I am not happy with the need of a (short running) poller and the select helper in C. Would be much better, if the pending data state could be evaluated within node itself. No clue if the stream API offers anything for that.
Up for review and merge...

@vlad-shatskyi
Copy link
Contributor

@Tyriar mentioning you just in case you haven't noticed it's ready for review.

@jerch
Copy link
Collaborator Author

jerch commented Jun 7, 2017

Removed the poller from Typescript, it is done now in the waitpid thread in C. I dont know how to do this with libuv, seems their IO poll API is to high level to get the "pending data state" without actually grabbing the data. Therefore with select.
The second slave file descriptor in the parent could potentially lead to a hanging fd if waitpid cant catch the child termination (e.g. SIGCHLD is not delivered). The parent will then occupy the pty device until it terminates itself, thus the system could run out of pty devices. Note this will only happen in rare cases, where the user undermines the SIGCHLD/waitpid functionality by purpose.
Still up for review...

Edit: I will have a closer look into libuv's uv_spawn, maybe there is a way to avoid low level select and the second slave fd.

@jerch
Copy link
Collaborator Author

jerch commented Jun 11, 2017

Weird, with my own much simpler node-pty impl in linux I cant reproduce the issue at all. The pty pipe always stays open as long as there is still data to be read (same with an even simpler pure C version). Really strange. Maybe libuv triggers the early closing of the pipe within the waitpid thread, no clue yet. Gonna need to dig though strace output to find out :(
Btw is there a reason why the module uses net.Socket and not tty.ReadStream as fd listener?

@Tyriar
Copy link
Member

Tyriar commented Jun 12, 2017

Btw is there a reason why the module uses net.Socket and not tty.ReadStream as fd listener?

@jerch do you mean here? https://github.com/Tyriar/node-pty/blob/4ad2860a48bb2980cfd4044c72346531bb578a9d/src/unixTerminal.ts#L254

Here's some relevant links for that chjj/pty.js#103, chjj/pty.js@2d45988

@jerch
Copy link
Collaborator Author

jerch commented Jun 13, 2017

@Tyriar Yep. Seems it is related to blocking/non-blocking mode of the tty streams in libuv. libuv puts blocking actions onto thread pool queues at a default poolsize of 4, therefore multiple blocking actions can lock each other. Not sure if this is still an issue in libuv with tty/pty, at least the doc mentions that libuv tries to open the fd nonblocking (see http://docs.libuv.org/en/v1.x/tty.html).

@vlad-shatskyi
Copy link
Contributor

@jerch, @Tyriar is there something blocking this PR from being merged?

@jerch
Copy link
Collaborator Author

jerch commented Jul 19, 2017

@vlad-shatskyi Imho this fix is not ready to merge since it introduces other issues. To get a decent fix this needs a closer look at the libuv sources, but I have no time for this atm :(

@jerch
Copy link
Collaborator Author

jerch commented Nov 3, 2018

Closing this PR as it is not a reliable solution with the additional open file descriptor.

@jerch jerch closed this Nov 3, 2018
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.

3 participants