Process hangs on node <= 14.1.0 and performance issue

[davidfirst]

I created a reproducible repo https://github.com/davidfirst/stream-issue-node-14-1-0 with instructions on how to reproduce.

In short, I run exec() and write data (string) to the ssh server through the stream.stdin. When the string is small (~1MB) it works with no issue. When the string is 10MB it works on node <=14.0.0 but it hangs on node >= 14.1.0.

Another issue I found, not related to any node version is the performance. The longer the string the lower the transfer rate. I have a feeling that it is related to the back-pressure, but I tried with piping as well, which suppose to handle the backpressure.

More details in the README of the repo above.

I didn't create two issues because there is a good chance that fixing the performance issue will fix the first issue as well.

(to have some more context for this issue, see teambit/bit#2844).

[mscdex]

I believe the issue is due to a regression in node since nodejs/node#32887.

[davidfirst]

Thanks @mscdex for the reply. How about the second issue? as you can see, the transfer rate decreases significantly with large data. This happens regardless of the node version I'm using.
Do you have any suggestions or a better way to handle the backpressure?

[davidfirst]

@mscdex , seems like there is no progress on the node repo. Best case they'll revert the change, but it'll be back on v15, so I'm not sure what's the plan from your side.
Are you planning on fixing it? If not, are you open to PRs? Is there any workaround I can use? If I'll change my implementation to use scp, would this solve the issue?

[mscdex]

The issue in question just happens to be worked around (AFAICT) in the rewrite I'm working on behind the scenes as it avoids using streams for internals as much as possible.

[davidfirst]

Thanks @mscdex , that's great news! We're looking forward to check it out.

[GiladShoham]

@mscdex do you have any kind of timeline or estimation about this internal changes?
Even a rough one?
Thanks!

[mscdex]

Soon™

[schmod]

Any hints about a potential workaround? Can we temporarily use a very large highWaterMark or something like that?

[mscdex]

If someone could test this with the current master branch, where the rewrite is in progress, and let me know if this issue still persists, that would be great.

[davidfirst]

@mscdex , I'd love to! The problem is that on master I'm unable to run npm install, it fails in the install.js script.
Here is the error:

➜  ssh2 git:(f763271) ✗ npm i

> ssh2@1.0.0-beta.0 install /Users/davidfirst/temp/projects/ssh2
> node install.js

  CXX(target) Release/obj.target/sshcrypto/src/binding.o
../src/binding.cc:1806:28: error: variable-sized object may not be initialized
    unsigned char calc_mac[hmac_len_] = {0};
                           ^~~~~~~~~
1 error generated.
make: *** [Release/obj.target/sshcrypto/src/binding.o] Error 1
gyp ERR! build error

I checked and the responsible commit is 632073f. Before this commit, I'm able to run npm install.

(Also, FYI, before getting this error, I got another error that I don't have cmake. I needed to brew install cmake to pass that error.).

[mscdex]

@davidfirst The error with cmake should not prevent installation of ssh2, as it comes from an optional dependency. Likewise with the compiler error, ssh2 should still be usable and should fall back to using node core crypto instead. The line number in the error is really strange though, as that does not match the problematic line shown....

Anyway, I've fixed the crypto binding compilation issue in 249a48c and fc68995.

[davidfirst]

Thanks @mscdex ! Both issues seem to be fixed now!! That's great news!
I made sure that I'm able to pass 300MB through SSH (it took around 2 seconds locally).
Also, the back-pressure seems to be resolved as the data transfer rate is stable around 150MBPS regardless of the data size.

Is there any ETA for a new version by any chance?

[mscdex]

@davidfirst No definite date. I think there's only a few things left to do/fix at the moment unless someone reports a serious bug/issue in the meantime. After those things are resolved I will probably do the release.

[schmod]

FYI, the underlying regression in Node appears to have been fixed in 15.1.0, and will likely be backported to the 14.x series.

[btimby]

Just to chime in with my experience. In case you need confirmation.

I had the exact same issue with file downloads, my implementation was slow and I rewrote it to make it faster. Once I had done that, it started to hang after a random interval, with the same debug output. I upgraded node to 15 and the problem went away, I now have fast downloads with no hangs.

[mscdex]

FWIW v1.0.0 is now released (and AFAIU should solve the problems detailed in here).