-
Notifications
You must be signed in to change notification settings - Fork 249
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
Port project to N-API #432
Conversation
@deepak1556 could you check this out when you get a chance? |
Tested on all three platforms 👍 |
|
A crash when an Electron renderer process gets reused. Looks like Node is forcefully stopping all AsyncWorkers during cleanup and an OnOK callback in node-pty crashes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eugeny the PR doesn't make the module multi-context aware but rather a refactor from NAN to NAPI.
LGTM for api changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this out and it seems to work initially, but when I create a 5th terminal in VS Code it doesn't work, and from that point on I cannot exit VS Code. It happens on both macOS and Windows (I didn't check Linux).
Steps to repro:
- Setup VS Code dev env
-
cd node_modules rmdir node-pty git clone https://github.com/DavidRusso/node-pty cd node-pty git checkout napi yarn
./scripts/code.bat
(or sh)
* | ||
* Ported to N-API by Matthew Denninghoff and David Russo | ||
* Reference: https://github.com/nodejs/node-addon-api | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add this to the big files ((con|win)?pty.cc
) if you want since this was a lot of effort, but it's a little overkill for the small ones. You'll be in commit history 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Same on Linux: Eugeny/tabby#3337 |
This may sound disruptive - I think the module should not be moved to the new native API without addressing the underlying bugs it already has (see #85 which reveals serious issues around the blocking semantics). To me it seems more appropriate to rewrite the whole module from scratch with the new API to get rid of those low level issues (and that awkward |
While I strongly agree that #85 is a serious issue, I don't think that it warrants delaying support for upcoming Electron versions once V8 API gets effectively banned. Even though above makes |
@Eugeny I admit that #85 has been around for ys, still the module is basically broken in this regard. A major API shift always opens the door to also get rid of those buried more serious issues, as you have to touch 80% of the code anyway. Note that it just happens to work for 90% of the module user by accident, because they normally use it with a shell and manage to attach a data handler fast enough. Still it shows nondeterministic behavior at these edges. |
Closing since this change has the remaining pretty big issue #432 (review). We do want to move to NAPI and may be force to eventually depending on Electron (if nan support breaks), that will make sharing binaries across platforms much nicer. |
* Port to NAPI The "5th pty bug" in #432 fixed also. * Fix help message in pty.cc * Move NAPI deps to devDependencies in package.json * Apply most of deepak1556's suggestions * Fix winpty * Fix conpty missing CloseHandle * Use unique_ptr to avoid `goto`s * Why macos failed? * fix: ci and minor cleanups * fix build failed on windows --------- Co-authored-by: deepak1556 <[email protected]>
commit 8bdbd71 Merge: 5063e0e d3ea906 Author: Daniel Imms <[email protected]> Date: Fri Oct 4 07:45:35 2024 -0700 Merge pull request microsoft#723 from microsoft/tyriar/compile_commands Move to compile commands on postinstall for fixing intellisense commit d3ea906 Author: Daniel Imms <[email protected]> Date: Fri Oct 4 07:30:55 2024 -0700 Move to compile commands on postinstall for fixing intellisense Fixes microsoft#707 commit 5063e0e Merge: 589a720 d06ed4f Author: Daniel Imms <[email protected]> Date: Thu Oct 3 07:27:53 2024 -0700 Merge pull request microsoft#722 from microsoft/tyriar/log Fix typo in trace log commit d06ed4f Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:56:29 2024 -0700 Fix typo in trace log Follow up microsoft#720 commit 589a720 Merge: de86139 353f219 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:33:40 2024 -0700 Merge pull request microsoft#721 from microsoft/tyriar/deps Move from yarn to npm commit de86139 Merge: b2d9866 53e60b0 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:24:35 2024 -0700 Merge pull request microsoft#719 from microsoft/tyriar/move_buffer_to_heap Move buffer_ to heap commit b2d9866 Merge: 1ece036 d2bf81f Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:22:56 2024 -0700 Merge pull request microsoft#718 from microsoft/tyriar/717 Close piClient.hThread handle commit 1ece036 Merge: 796e11e 7e431b9 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:20:46 2024 -0700 Merge pull request microsoft#720 from microsoft/tyriar/catch_winpty_deconstructor Catch possible exceptions in ~Agent commit 353f219 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:20:09 2024 -0700 Regenerate dependencies commit 13c661d Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:12:07 2024 -0700 yarn.lock -> package-lock.json Fixes microsoft#712 commit 7e431b9 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:07:45 2024 -0700 Catch possible exceptions in ~Agent Fixes warning: deps\winpty\src\agent\Agent.cc(231): warning C4722: 'Agent::~Agent': destructor never returns, potential memory leak commit 53e60b0 Author: Daniel Imms <[email protected]> Date: Thu Oct 3 06:00:25 2024 -0700 Move buffer_ to heap Resolves warning: src\win\path_util.cc(54): warning C6262: Function uses '131804' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap. commit d2bf81f Author: Daniel Imms <[email protected]> Date: Thu Oct 3 05:54:52 2024 -0700 Close piClient.hThread handle Fixes microsoft#717 commit 796e11e Merge: cb94da7 59771c0 Author: Daniel Imms <[email protected]> Date: Fri Sep 27 07:19:10 2024 -0700 Merge pull request microsoft#713 from microsoft/tyriar/711 Bring back clear impl commit 59771c0 Author: Daniel Imms <[email protected]> Date: Mon Sep 9 06:28:10 2024 -0700 Bring back clear impl Fixes microsoft#711 commit cb94da7 Merge: 8c0a6d7 4f2f641 Author: Daniel Imms <[email protected]> Date: Sun Aug 11 09:34:35 2024 -0700 Merge pull request microsoft#704 from microsoft/tyriar/npm_arch Copy right dll/exe based on npm_config_arch commit 4f2f641 Author: Daniel Imms <[email protected]> Date: Sun Aug 11 08:33:05 2024 -0700 Copy right dll/exe based on npm_config_arch Part of microsoft/vscode#224488 Part of microsoft/vscode#225355 commit 8c0a6d7 Merge: 8fc21c9 ef1cefc Author: Daniel Imms <[email protected]> Date: Fri Aug 9 09:09:55 2024 -0700 Merge pull request microsoft#703 from microsoft/tyriar/conpty_prefix Fix correct method being loaded when not using conptydll commit ef1cefc Author: Daniel Imms <[email protected]> Date: Fri Aug 9 08:37:52 2024 -0700 Fix correct method being loaded when not using conptydll Part of microsoft/vscode#224488 commit 8fc21c9 Merge: 7a8c67c 061487a Author: Daniel Imms <[email protected]> Date: Tue Aug 6 14:19:47 2024 -0700 Merge pull request microsoft#702 from microsoft/tyriar/conpty_update Update to newer version of conpty commit 061487a Author: Daniel Imms <[email protected]> Date: Tue Aug 6 13:56:45 2024 -0700 Update to newer version of conpty commit 7a8c67c Merge: 485379c cf62104 Author: Daniel Imms <[email protected]> Date: Tue Aug 6 05:36:19 2024 -0700 Merge pull request microsoft#700 from microsoft/tyriar/conpty_path Get path of conpty.node for conpty.dll commit cf62104 Author: Daniel Imms <[email protected]> Date: Tue Aug 6 05:32:14 2024 -0700 Get path of conpty.node for conpty.dll Part of microsoft/vscode#224488 commit 485379c Merge: 1247f91 98d8566 Author: Daniel Imms <[email protected]> Date: Mon Aug 5 09:04:40 2024 -0700 Merge pull request microsoft#699 from microsoft/tyriar/conptydll_exception Improve can't find conpty.dll exception message commit 98d8566 Author: Daniel Imms <[email protected]> Date: Mon Aug 5 08:21:32 2024 -0700 Improve can't find conpty.dll exception message Part of microsoft/vscode#224488 commit 1247f91 Merge: 47c16e2 5e99f76 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 13:30:11 2024 -0700 Merge pull request microsoft#698 from microsoft/tyriar/publish_third_party Publish third_party folder commit 5e99f76 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 13:22:55 2024 -0700 Publish third_party folder commit 47c16e2 Merge: 5874200 59f5d47 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 12:41:41 2024 -0700 Merge pull request microsoft#697 from microsoft/tyriar/conpty4 Disable APIScan and reduce test flakiness on Windows commit 59f5d47 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 11:48:07 2024 -0700 Fix winpty resize and reduce test flakiness commit 055cd3f Author: Daniel Imms <[email protected]> Date: Thu Aug 1 11:38:28 2024 -0700 Disable APIScan task commit 5874200 Merge: 2622bd1 dde63ec Author: Daniel Imms <[email protected]> Date: Thu Aug 1 11:25:33 2024 -0700 Merge pull request microsoft#696 from microsoft/tyriar/node16 Don't run CI for node 16 commit dde63ec Author: Daniel Imms <[email protected]> Date: Thu Aug 1 11:22:01 2024 -0700 Don't run CI for node 16 commit 2622bd1 Merge: bd7fa4f 7c4e9ed Author: Daniel Imms <[email protected]> Date: Thu Aug 1 10:47:54 2024 -0700 Merge pull request microsoft#695 from microsoft/tyriar/conpty2 Ship third_party folder with module commit 7c4e9ed Author: Daniel Imms <[email protected]> Date: Thu Aug 1 10:24:27 2024 -0700 Ship third_party folder with module This needs to be there to copy over the right binary on postinstall Part of microsoft/vscode#224488 commit bd7fa4f Merge: efbf8eb 4392169 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 09:55:36 2024 -0700 Merge pull request microsoft#694 from microsoft/tyriar/conptydll Ship conpty.dll/OpenConsole.exe with opt-in experimental option useConptyDll commit 4392169 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 08:33:27 2024 -0700 Copy conpty.dll and openconsole.exe depending on arch in postinstall commit ddcd0b0 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 08:14:27 2024 -0700 Add useConptyDll option commit 95f67e7 Author: Daniel Imms <[email protected]> Date: Thu Aug 1 08:11:38 2024 -0700 Add conpty dlls and node headers commit f2be1af Author: Daniel Imms <[email protected]> Date: Tue Jul 30 11:45:51 2024 -0700 Get conpty.dll conditionally loading commit efbf8eb Merge: 8be08fd 0e791c1 Author: Daniel Imms <[email protected]> Date: Tue Jun 18 05:55:56 2024 -0700 Merge pull request microsoft#687 from microsoft/dependabot/npm_and_yarn/braces-3.0.3 build(deps): bump braces from 3.0.2 to 3.0.3 commit 8be08fd Merge: df1ae8c 26f1c69 Author: Daniel Imms <[email protected]> Date: Mon Jun 17 07:35:47 2024 -0700 Merge pull request microsoft#688 from jpcastberg/termios-freebsd Include termios.h on freebsd commit 26f1c69 Author: jpcastberg <[email protected]> Date: Sun Jun 16 23:14:24 2024 -0700 Include termios.h on freebsd commit 0e791c1 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun Jun 16 11:12:19 2024 +0000 build(deps): bump braces from 3.0.2 to 3.0.3 Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3. - [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md) - [Commits](micromatch/braces@3.0.2...3.0.3) --- updated-dependencies: - dependency-name: braces dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> commit df1ae8c Author: Raymond Zhao <[email protected]> Date: Tue Apr 16 15:43:10 2024 -0700 docs: add a note on Spectre-mitigated libraries (microsoft#679) commit f014e86 Merge: 6aa0758 0fbbcf6 Author: Daniel Imms <[email protected]> Date: Thu Apr 11 06:45:41 2024 -0700 Merge pull request microsoft#678 from microsoft/dependabot/npm_and_yarn/tar-6.2.1 build(deps): bump tar from 6.2.0 to 6.2.1 commit 0fbbcf6 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Apr 11 12:09:54 2024 +0000 build(deps): bump tar from 6.2.0 to 6.2.1 Bumps [tar](https://github.com/isaacs/node-tar) from 6.2.0 to 6.2.1. - [Release notes](https://github.com/isaacs/node-tar/releases) - [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md) - [Commits](isaacs/node-tar@v6.2.0...v6.2.1) --- updated-dependencies: - dependency-name: tar dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> commit 6aa0758 Author: Raymond Zhao <[email protected]> Date: Mon Apr 1 11:37:15 2024 -0700 chore: migrate pipeline to use 1ES template (microsoft#676) commit 3913479 Author: kkocdko <[email protected]> Date: Mon Mar 18 21:01:50 2024 +0800 chore: remove deprecated api `process.binding` (microsoft#653) * Remove deprecated API `process.binding` Originally designed to work with ancient node.js 0.12 and io.js * Fix node `net.Socket` limitations nodejs/node#37780 commit f25bb55 Author: kkocdko <[email protected]> Date: Sat Mar 16 00:26:59 2024 +0800 Upgrade node-gyp to fix microsoft#643 and microsoft#646 (microsoft#673) commit 783c7ed Author: Robo <[email protected]> Date: Thu Mar 7 22:25:19 2024 +0900 fix: assertion on node environment shutdown (microsoft#672) commit 2dab380 Author: Raymond Zhao <[email protected]> Date: Mon Mar 4 07:15:19 2024 -0800 fix: comment out invalid API call (microsoft#669) commit c712cce Author: Raymond Zhao <[email protected]> Date: Thu Feb 29 11:55:21 2024 -0800 chore: fix APIScan software name (microsoft#667) commit a0dd31d Merge: 0661eaf 0253460 Author: SteVen Batten <[email protected]> Date: Sun Feb 25 15:21:58 2024 -0800 Merge pull request microsoft#666 from microsoft/sbatten/apiscan api scan commit 0253460 Author: SteVen Batten <[email protected]> Date: Sun Feb 25 14:59:02 2024 -0800 fix job name commit 2483032 Author: SteVen Batten <[email protected]> Date: Sun Feb 25 14:57:25 2024 -0800 api scan commit 0661eaf Merge: b1fdda4 e08593d Author: Daniel Imms <[email protected]> Date: Wed Feb 21 07:39:05 2024 -0800 Merge pull request microsoft#664 from microsoft/dependabot/npm_and_yarn/ip-2.0.1 build(deps): bump ip from 2.0.0 to 2.0.1 commit e08593d Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed Feb 21 01:42:29 2024 +0000 build(deps): bump ip from 2.0.0 to 2.0.1 Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1. - [Commits](indutny/node-ip@v2.0.0...v2.0.1) --- updated-dependencies: - dependency-name: ip dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> commit b1fdda4 Author: kkocdko <[email protected]> Date: Fri Jan 26 17:17:03 2024 +0800 Port to NAPI (microsoft#644) * Port to NAPI The "5th pty bug" in microsoft#432 fixed also. * Fix help message in pty.cc * Move NAPI deps to devDependencies in package.json * Apply most of deepak1556's suggestions * Fix winpty * Fix conpty missing CloseHandle * Use unique_ptr to avoid `goto`s * Why macos failed? * fix: ci and minor cleanups * fix build failed on windows --------- Co-authored-by: deepak1556 <[email protected]>
Is there any interest in porting the project from NAN to N-API? If so, here is a working port.