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.

Sign up for GitHub

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

Jump to bottom

SPDY fails in node >= 11.1.0 #350

Closed
bhevesi opened this issue Nov 4, 2018 · 42 comments
Closed

SPDY fails in node >= 11.1.0 #350

bhevesi opened this issue Nov 4, 2018 · 42 comments
Labels
bug

Comments

@bhevesi
Copy link

bhevesi commented Nov 4, 2018

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 jacobheun changed the title SPDY blocked works fine by "handle-thing": "^1.2.5" SPDY fails in node >= 11.1.0 Nov 8, 2018
@jacobheun
Copy link
Contributor

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.

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.

@jacobheun jacobheun added the bug label Nov 8, 2018
@bhevesi
Copy link
Author

bhevesi commented Nov 9, 2018

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

@bhevesi bhevesi closed this as completed Nov 9, 2018
@bhevesi
Copy link
Author

bhevesi commented Nov 9, 2018 via email

@p3x-robot
Copy link

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
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@bhevesi
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@jacobheun
Copy link
Contributor

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

Reopening this.

@jacobheun jacobheun reopened this Nov 11, 2018
@bhevesi
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@jacobheun
Copy link
Contributor

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
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@p3x-robot
Copy link

@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
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@p3x-robot
Copy link

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

@bhevesi
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@p3x-robot
Copy link

Ok, i hang on then. Thanks

@bhevesi
Copy link
Author

bhevesi commented Nov 11, 2018 via email

@jacobheun
Copy link
Contributor

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, spdy-http2/handle-thing#5 (comment). 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
Copy link

@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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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 p3x-robot mentioned this issue Nov 19, 2018
Closed
3 tasks
@p3x-robot
Copy link

nodejs/node#23797 (comment)

@p3x-robot
Copy link

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. 👍

image

@p3x-robot
Copy link

@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
Copy link
Contributor

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+.

This was referenced Dec 7, 2018
Closed
Closed
@AdamMagaluk AdamMagaluk mentioned this issue Dec 11, 2018
Open
4 tasks
@p3x-robot
Copy link

p3x-robot commented Jan 1, 2019 •
edited
Loading

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
Copy link

yi-ge commented Feb 11, 2019

@jacobheun Thank you very much.

@yi-ge yi-ge mentioned this issue Feb 11, 2019
Merged
6 tasks
@bernardbr bernardbr mentioned this issue Feb 13, 2019
Open
@gomesNazareth
Copy link

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

@tamasszoke tamasszoke mentioned this issue Apr 15, 2019
Closed
@espadrine espadrine mentioned this issue May 9, 2019
Merged
espadrine added a commit to espadrine/checksum-user-study that referenced this issue May 9, 2019
@espadrine
Disable SPDY
7c2cc1b 
This solves the following issue: spdy-http2/node-spdy#350
@LarsDenBakker LarsDenBakker mentioned this issue May 24, 2019
Closed
@tomalex0
Copy link

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

@p3x-robot
Copy link

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

@boompig boompig mentioned this issue Aug 9, 2019
Open
waffledonkey referenced this issue in spdy-http2/handle-thing Dec 14, 2019
@Cellule
fix: support onStreamRead for node 11.1 and above
4e4aae8
bitifet added a commit to bitifet/exposito that referenced this issue Jan 10, 2020
@bitifet
First ssl real try and documentation update.
1982e0b 
Seems to not working under node versions over 11.1.0 because of spdy
bug:

spdy-http2/node-spdy#350

Tested to work on node v10.16.3

Previous commits should work too (under node versions prior to 11.1.0).
Code were ported from a working project and there is no reason for it to
fail.

This commit only improves error handling and completes documentation.
@aaclayton
Copy link

aaclayton commented Mar 25, 2020 •
edited
Loading

The necessary change has been reviewed and merged in the upstream handle-thing package: spdy-http2/handle-thing#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
Copy link

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.

@cesarchefinho cesarchefinho mentioned this issue Mar 26, 2020
Open
@mir4ef
Copy link

mir4ef commented Mar 31, 2020 •
edited
Loading

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
Copy link

Aymkdn commented Apr 3, 2020 •
edited
Loading

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 [email protected] … I also changed the handle-thing version in my root package-lock.json…

@mir4ef
Copy link

mir4ef commented Apr 4, 2020

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?

@indutny indutny closed this as completed in a95ca58 Apr 4, 2020
@Aymkdn
Copy link

Aymkdn commented Apr 6, 2020

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
Copy link

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

@studentIvan
Copy link

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

@aphix
Copy link

aphix commented May 18, 2020 •
edited
Loading

It looks like this issue was closed by a95ca58. But looking at the package.json diff, it's still using [email protected]. 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:
a95ca58#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:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

No branches or pull requests