SPDY fails in node >= 11.1.0

[bhevesi]

Hi,

Couple of short investigation, why SPDY drop error message //------------------------------------------------------------------------------------------------------- buffer.js:72 class FastBuffer extends Uint8Array {} ^

RangeError: Invalid typed array length: -104 at new Uint8Array () at new FastBuffer (buffer.js:72:1) at Handle.onStreamRead [as onread] (internal/stream_base_commons.js:121:17) at Immediate. (/home/bhevesi/node_modules/handle-thing/lib/handle.js:128:16) at processImmediate (timers.js:632:19) //------------------------------------------------------------------------------------------------------- using new version (11) of node.js.

I found that node internal/stream_base_commons.js has been changed in new version but the SPDY dependency ("handle-thing": "^1.2.5") is not properly call it in /lib/handles.js file.

In Node, function onStreamRead(arrayBuffer) called from handles.js via self.onread(uv.UV_EOF, new Buffer(0)) and self.onread(uv.UV_ECONNRESET, new Buffer(0)). So the given two parameters (instead of one) generates error.

In order to eliminate these errors, I had some changes in /lib/handles.js file: Replaced "self.onread(uv.UV_EOF, new Buffer(0))" to self.onread(new Buffer(0)) and self.onread(uv.UV_ECONNRESET, new Buffer(0)) to self.onread(new Buffer(0))

Looks errors are disappears but more deep investigation required or/and live maintenance of "handle-thing": "^1.2.5" module

Best regards: Bela

[jacobheun]

We've upgraded spdy to work with node LTS version (6,8,10). This is due to a recent stream change in node 11.1. Handle thing will need to be updated https://github.com/spdy-http2/handle-thing/issues/6.

I did some quick updates to onread to see if I could resolve the problem for the handle-thing 2.0.0 release, but there are other internal stream changes that will need to be taken into account. Read is leveraging the internal stream state, so we'll need to make sure handle thing is managing that as needed.

[bhevesi]

I appreciate Your change, warm welcome and thank You so much!

[bhevesi]

Dear Jacob,

Many-many thanks for Your update. I appreciate Your hard work, quick reply, I wish You the best!

Have a nice night!

Best regards, Bela

Jacob Heun notifications@github.com ezt írta (időpont: 2018. nov. 8., Cs, 11:05):

We've upgraded spdy to work with node LTS version (6,8,10). This is due to a recent stream change in node 11.1. Handle thing will need to be updated spdy-http2/handle-thing#6 https://github.com/spdy-http2/handle-thing/issues/6.

I did some quick updates to onread to see if I could resolve the problem for the handle-thing 2.0.0 release, but there are other internal stream changes that will need to be taken into account. Read is leveraging the internal stream state, so we'll need to make sure handle thing is managing that as needed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-436940671, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj70Ak5SI8gSJrYwJ33ojYqSM3cYUks5utAH7gaJpZM4YNQkE .

[p3x-robot]

hello, how are ya? i am not understanding why this bug is closed, when it is not resolved,? we always using the current node version. could you please tell me what is going on? are we waiting for an upstream package to be fixed?

thanks!

[bhevesi]

Hi,

You are correct! Apologize, I closed this issue without deep check of compatibility with Node 11.1.0 version, my mistake, sorry for that.

I see, further fixes are required. In order to eliminate the current error message, I have changed two lines in handle.js (version 2.0.0) file:

1. line #88:  self.onread(uv.UV_EOF, Buffer.alloc(0)) TO  self.onread(

Buffer.alloc(0))

2. line #94: self.onread(uv.UV_ECONNRESET, Buffer.alloc(0)) TO self. onread(Buffer.alloc(0))

I don't know how to reopen this issue, may open a new one? What do You think?

Best regards, Bela

patrikx3 notifications@github.com ezt írta (időpont: 2018. nov. 10., Szo, 10:07):

hello, how are ya? i am not understanding why this bug is closed, when it is not resolved,? we always using the current node version. could you please tell me what is going on? are we waiting for an upstream package to be fixed?

thanks!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437569758, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj_56upWFFVV6YngyIlzn7JqhdhOVks5utpdqgaJpZM4YNQkE .

[bhevesi]

Dear Jacob,

You are correct! Apologize, I closed this issue without deep check of compatibility with Node 11.1.0 version, my mistake, sorry for that.

I see, further fixes are required. In order to eliminate the current error message, I have changed two lines in handle.js (version 2.0.0) file:

1. line #88:  self.onread(uv.UV_EOF, Buffer.alloc(0)) TO  self.onread(

Buffer.alloc(0))

2. line #94: self.onread(uv.UV_ECONNRESET, Buffer.alloc(0)) TO self. onread(Buffer.alloc(0))

I don't know how to reopen this issue, may open a new one? What do You think?

Best regards, Bela

Jacob Heun notifications@github.com ezt írta (időpont: 2018. nov. 8., Cs, 11:05):

We've upgraded spdy to work with node LTS version (6,8,10). This is due to a recent stream change in node 11.1. Handle thing will need to be updated spdy-http2/handle-thing#6 https://github.com/spdy-http2/handle-thing/issues/6.

I did some quick updates to onread to see if I could resolve the problem for the handle-thing 2.0.0 release, but there are other internal stream changes that will need to be taken into account. Read is leveraging the internal stream state, so we'll need to make sure handle thing is managing that as needed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-436940671, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj70Ak5SI8gSJrYwJ33ojYqSM3cYUks5utAH7gaJpZM4YNQkE .

[jacobheun]

Yes, this is still an issue with handle-thing and is being tracked here: https://github.com/spdy-http2/handle-thing/issues/6. The most recent update I did was to get everything working on LTS first.

Reopening this.

[bhevesi]

Thanks again! Do You reopening or me to do it somehow?

Jacob Heun notifications@github.com ezt írta (időpont: 2018. nov. 11., V, 14:20):

Yes, this is still an issue with handle-thing and is being tracked here: spdy-http2/handle-thing#6 https://github.com/spdy-http2/handle-thing/issues/6. The most recent update I did was to get everything working on LTS first.

Reopening this.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437669735, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvjysztwVcfdbCawV45PPjvcXElipQks5uuCQggaJpZM4YNQkE .

[jacobheun]

I've reopened the issue. I'm not sure when I will have time to fix >=11.1.0 support, but I am happy to review any PRs submitted if someone beats me to it!

[bhevesi]

:-) I don not want to beat You, never ever :-)

I'm thankful for Your help!

Jacob Heun notifications@github.com ezt írta (időpont: 2018. nov. 11., V, 14:38):

I've reopened the issue. I'm not sure when I will have time to fix

=11.1.0 support, but I am happy to review any PRs submitted if someone beats me to it!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437670929, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj5-VwzxTtxNUl89Qa-6i-ULRxJyzks5uuCg8gaJpZM4YNQkE .

[p3x-robot]

@bhevesi so are you gonna create a PR to fix this bug or should I do it? Just have to change these 2 lines?

[bhevesi]

Not necessary, Jacob Heun knows it and working on fix the issue. Thanks a lot!

patrikx3 notifications@github.com ezt írta (időpont: 2018. nov. 11., V, 14:51):

@bhevesi https://github.com/bhevesi so are you gonna create a PR to fix this bug or should I do it? Just have to change these 2 lines?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437671827, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj0NV6U5_NJZJZLQ7W83lJt-zZcdIks5uuCtHgaJpZM4YNQkE .

[p3x-robot]

jacobheun says he is not sure when he gonna fix this.

[bhevesi]

Couple of minutes ago I changed a few mails with him. The issue has been reopened. Jacob knows my changes to eliminate this issue. So I hope the final solution is not so far. Is it OK for You?

patrikx3 notifications@github.com ezt írta (időpont: 2018. nov. 11., V, 15:02):

jacobheun says he is not sure when he gonna fix this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437672665, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvj6rdEE3riXqFtQIw3aa9tTF_Ylwyks5uuC4RgaJpZM4YNQkE .

[p3x-robot]

Ok, i hang on then. Thanks

[bhevesi]

I would like to thanks for You!

patrikx3 notifications@github.com ezt írta (időpont: 2018. nov. 11., V, 15:09):

Ok, i hang on then. Thanks

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spdy-http2/node-spdy/issues/350#issuecomment-437673216, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNvjyCLXIboysTRPaKFcq9tdMCowqtjks5uuC-sgaJpZM4YNQkE .

[jacobheun]

Sorry if there's confusion here, bu @p3x-robot is correct, I am not sure when I will get to this. There is more that needs to change aside from those 2 lines. We'll need to do some node version checking, and fix other issues. handle-thing tests don't pass with just those changes on node 11.1. The updates may need to make sure that the internal stream state is being set correctly as the change in node 11.1.0 leverages that more. I linked out to the internal file in the last PR I submitted for the LTS changes in handle-thing, https://github.com/spdy-http2/handle-thing/pull/5#issue-229119787. The changes to that file would be a good thing to review for anyone who is interested in submitted a fix.

If I do start working on this, I will post an update here.

[p3x-robot]

@jacobheun so what does we have to do eg:

• change for the 2 lines
  • with a if, if it is above/equals v11.1.0
  • if not above/equals v11.1.0 keep the original settings
• what else to change?

[jacobheun]

I'm not certain on the full changes needed, I didn't have enough time to dive into it, but just changing those lines still fails tests on node 11.1.

[p3x-robot]

so basically the 2 lines make it to work and then tests failed only, right? since the guy said if we change the 2 lines it works, and then only the tests should be fixed is that correct?

[jacobheun]

No, the tests should not need to be touched. There is more code that needs to be updated in order to get the tests passing in both the handle-thing and node-spdy repos.

[p3x-robot]

so you have no idea when it can be fixed in the current release nodejs? dont you have some pointers to if where i should go to create a PR?

[jacobheun]

In order to understand what needs to be fixed in handle-thing you have to know what changed in node 11.1.0. Node 11.0.0 is fine, but 11.1.0 introduced the change to stream internals. Since the internals are not documented, you either have to check out the code changes, or reach out to the node team. If you check the commit log and PR history there are some key changes that should be checked, like https://github.com/nodejs/node/pull/23797/files. This also has examples of what they changed in some of the node consumer handlers, like net, to make sure that's working.

I would start at those changes and then work to replicate the needed changes in handle-thing to get tests passing there. Then, verify that tests pass here with those changes.

I haven't had time to dive into the node changes, which is why I'm not certain of the full extent of changes needed.

[p3x-robot]

https://github.com/nodejs/node/pull/23797#issuecomment-439875742

[p3x-robot]

Ok, but I checked the code and it is exactly the 2 lines changed. The signature of those functions changed. The signature is 1 parameter now and all we need to do is to check the Node version and if it is SEMVER CHECK >= NodeJs V11.1.0, then the parameter is different. I suppose your test would be different as well. There is no other way than changing the onread function... If we do not change the test it will fail for sure. 👍

[addaleax]

@p3x-robot The nread value is still there, but it’s not passed as a function argument, but rather through an internal typed array. Those values are currently accessible through the deprecated process.binding('stream_wrap') – you could fix up modules that hook into Node’s internals to work with that, for now.

However, as stated in https://github.com/spdy-http2/node-spdy/issues/333, the spdy module relies on Node’s internals to such a degree that it’s unreasonable to expect it to keep working on Node 10+.

If y’all fix it up so that it works on Node 11, we could re-introduce it into CITGM, our tool that runs tests of popular or important packages on Node.js releases. But please, know that that is a temporary solution – in the long term, either spdy needs to be re-written, or (better) you should migrate to the built-in http2 module.

[p3x-robot]

@jacobheun given spdy is using heavy internal functions which is bad, can't we just use the native http2 as it is in the latest nodejs since v10?

[jacobheun]

or (better) you should migrate to the built-in http2 module.

I agree with @addaleax, if you can upgrade your applications to use http2 that's the way to go. The SPDY protocol has been deprecated in favor of http2. It doesn't make much sense to rework this module to improve long term maintainability with http2 out, and http3 likely happening sometime next year.

Personally I'd rather see ongoing efforts here focused on maintenance/support for node (6-10) and a migration guide to http2 for node 11+.

[p3x-robot]

i solved it with pure http and used nginx as the proxy using http2, i think the fastest http2 solution (nodejs http2 is quite slow)

[yi-ge]

@jacobheun Thank you very much.

[gomesNazareth]

I was facing the same issue. For a quick solution . I just downgraded node. sudo npm cache clean -f sudo npm install -g n sudo n 10.6.0

[tomalex0]

@p3x-robot do you have sample code to in nginx to enable http2, where you able to do file push from nginx ?

[p3x-robot]

@tomalex0 sorry, i do not use http2 push, i use socket.io ...

[aaclayton]

The necessary change has been reviewed and merged in the upstream handle-thing package: https://github.com/spdy-http2/handle-thing/pull/13

The next step is to use the new handle-thing 2.0.1 version in node-spdy and this issue should (hopefully) be resolved!

[cesarchefinho]

This issue comes from oct/2018 and we are in midle of 2020. Please don't wait and merge handle-thing 201 into spdy. A lot of users will be glad.

[mir4ef]

after bumping handle-thing (with npm update --depth=1 handle-thing since it is a dep of spdy) it works for me on node v12.

[Aymkdn]

npm update --depth=1 handle-thing

It didn't work for me, even with a deeper number. I have an old project and I didn't want to delete node_modules and reinstall everything.

What I did: I went in node_modules/spdy/ and I executed npm install handle-thing@2.0.1 … I also changed the handle-thing version in my root package-lock.json…

[mir4ef]

when you perform these steps, does handle-thing get updated? what is the version inside package.json inside the handle-thing folder? does your lock file get updated to indicate it picked up the update? do you have another package that might be depending on handle-thing and maybe using an older version (<2)?

if you dont want to remove the entire node_modules folder, you can try to uninstall spdy and then reinstall it.

also, what is your version of npm?

[Aymkdn]

when you perform these steps, does handle-thing get updated?

If by "these steps" you mean the steps I described, then the answer is Yes.

what is the version inside package.json inside the handle-thing folder?

I don't have ./node_modules/handle-thing, but only ./node_modules/spdy/node_modules/handle-thing, and the version is now 2.0.1

does your lock file get updated to indicate it picked up the update?

I manually updated it.

do you have another package that might be depending on handle-thing and maybe using an older version (<2)?

No.

if you dont want to remove the entire node_modules folder, you can try to uninstall spdy and then reinstall it.

No need as my workaround did the trick.

also, what is your version of npm?

It's currently 6.14.4, but this project is 2 or 3 years old, so I don't know which version I used at that time to install spdy :-)

It's all good now anyway! Thanks.

[derrickrung]

It looks like this issue was closed by https://github.com/spdy-http2/node-spdy/commit/a95ca58377cd21ea8c03498e81bfcd99a5f1867f. But looking at the package.json diff, it's still using handle-thing@2.0.0. I'm not able to get this working on later versions of Node (12.13.0, 13.12.0). Any update on this?

[studentIvan]

This gist can help somebody who needs to use spdy only for development http2 server with express js

[aphix]

It looks like this issue was closed by a95ca58. But looking at the package.json diff, it's still using handle-thing@2.0.0. I'm not able to get this working on later versions of Node (12.13.0, 13.12.0). Any update on this?

@derrickrung,

The package.json has ^2.0.0 which means that 2.0.1 will be resolved as it satisfies the ^ caret at the beginning.

In the commit you linked, this can be seen in diff in the package-lock.json here: https://github.com/spdy-http2/node-spdy/commit/a95ca58377cd21ea8c03498e81bfcd99a5f1867f#diff-32607347f8126e6534ebc7ebaec4853dL1047-R1046

What package manager are you using? I've tested and it's working fine with both npm and pnpm.

Edit: Here's a screenshot to the package-lock.json diff since github doesn't like to auto-expand to large diffs: