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

fix(Server): don't use spdy on node >= v10.0.0 #1451

Merged
merged 2 commits into from Aug 29, 2018

Conversation

addaleax
Copy link
Contributor

@addaleax addaleax commented Aug 1, 2018

  • This is a bugfix
  • This is a code refactor
  • This is a test update
  • This is a typo fix
  • This is a metadata update

For Bugs and Features; did you add new tests?

This does add Node 10 to the test matrix. There do not seem to be any existing tests for https:// support, though, so I added one, even though it does not function as a regression test (one would actually have to explicitly use spdy/http2 for that).

Motivation / Use-Case

This fixes the bug reported at:

Breaking Changes

This should not affect any user agents that detect protocol support themselves (like they should), in particular not browsers.

Additional Info

Another option that was considered was reverting the change that led to this in Node 10.

However, the spdy module is built in a very fragile manner, and unfortunately it seems unreasonable to expect that anything we can do inside of Node would be more than a stop-gap solution.

(See e.g. spdy-http2/node-spdy#333 for an attempt to reach out to the spdy maintainers about this problematic.)

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Aug 1, 2018

/cc @SpaceK33z

@codecov
Copy link

@codecov codecov bot commented Aug 1, 2018

Codecov Report

Merging #1451 into master will increase coverage by 0.72%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
+ Coverage    73.3%   74.02%   +0.72%     
==========================================
  Files          10       10              
  Lines         663      666       +3     
==========================================
+ Hits          486      493       +7     
+ Misses        177      173       -4
Impacted Files Coverage Δ
lib/Server.js 81.44% <100%> (+1.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4740224...cf3374b. Read the comment docs.

@addaleax
Copy link
Contributor Author

@addaleax addaleax commented Aug 1, 2018

Updated the test with an increased timeout because of the on-the-fly key generation.

(Edit: And updated again to appease the linter.)

@Jessidhia
Copy link
Member

@Jessidhia Jessidhia commented Aug 1, 2018

Unfortunately, just changing the module to http2 won't work because of expressjs/express#3388

Express doesn't seem to actually use node's request/response stuff but uses custom ones which are then setPrototypeOf to the node versions, and because of that it actually will not work correctly and crashes as soon as an h2 request happens.

@addaleax
Copy link
Contributor Author

@addaleax addaleax commented Aug 1, 2018

@Kovensky Without having looked into it deeper, from expressjs/express#3390 (comment) and below, it looks like there might be some movement in that area? :)

@addaleax
Copy link
Contributor Author

@addaleax addaleax commented Aug 2, 2018

Btw, are the recommendations for trying out pull requests in the CONTRIBUTING.md accurate? See e.g. nodejs/node#21665 (comment)

@leup
Copy link

@leup leup commented Aug 15, 2018

Any news on this one ?

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Aug 15, 2018

/cc @Kovensky this PR don't fix problem? Can i look on original issue and/or minimum reproducible test repo?

@michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Aug 15, 2018

I think this is definitely blocked by nodejs/node#21665 and expressjs/express#3388 atm

@Jessidhia
Copy link
Member

@Jessidhia Jessidhia commented Aug 16, 2018

@evilebottnawi this branch itself should reproduce it; the hard part is automated testing.

To reproduce it, just do an h2 connection to webpack-dev-server in --https mode.


@michael-ciniawsky "fixing" the node bug will just make the spdy module stop crashing but is not the real fix; either fixing the express bug or migrating away from express are.

@michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Aug 16, 2018

@Kovensky Yep, I understand it's blocked on that :)

@addaleax
Copy link
Contributor Author

@addaleax addaleax commented Aug 18, 2018

I think this is definitely blocked by nodejs/node#21665 and expressjs/express#3388 atm

@michael-ciniawsky I’m not sure if I’m understanding you correctly, but this PR is not blocked on either of these issues. It fixes the first one, and the second one is just something to keep in mind for the future (fully migrating from SPDY to the built-in HTTP/2 module in Node would be dependent on it being solved).

What this PR may be blocked on is this issue, reported by somebody who tried this code out: nodejs/node#21665 (comment)

I don’t know enough about webpack-dev-server to figure out whether that issue is actually related, and if so, in what way. (And as @Kovensky said, automated testing is hard here & I’m not sure how to do it well.)

@michael-ciniawsky michael-ciniawsky changed the title Do not use spdy on Node 10 fix(Server): don't use spdy on node >= v10.0.0 Aug 21, 2018
@michael-ciniawsky michael-ciniawsky added this to the 3.1.6 milestone Aug 21, 2018
@michael-ciniawsky michael-ciniawsky self-requested a review Aug 21, 2018
`spdy` is effectively unmaintained, and as a consequence of an
implementation that extensively relies on Node’s non-public APIs, broken
on Node 10 and above. In those cases, only https will be used for now.
Once express supports Node's built-in HTTP/2 support, migrating over to
that should be the best way to go.

Fixes: webpack#1449
Fixes: nodejs/node#21665
@addaleax
Copy link
Contributor Author

@addaleax addaleax commented Aug 28, 2018

I’ve rebased, and dropped the commit that adds Node 10 to the CI matrix since the .travis.yml has changed significantly (and I assume it would have been added anyway as part of that refactor if there was interest in it).

eagerdev12 added a commit to eagerdev12/creating-app that referenced this issue Dec 27, 2021
This fixes the functionality of this dependency for Node 10 and above.

Refs: webpack/webpack-dev-server#1451
Refs: nodejs/node#21665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants