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

spdy issue #336

Closed
marvinroger opened this issue Apr 8, 2017 · 24 comments · Fixed by #379 · May be fixed by #373
Closed

spdy issue #336

marvinroger opened this issue Apr 8, 2017 · 24 comments · Fixed by #379 · May be fixed by #373

Comments

@marvinroger
Copy link
Contributor

Hi,

The following:

const zetta = require('zetta')

zetta()
  .name('Test')
  .listen(1337, function () {
    console.log('Zetta is running')
  })

Fails beautifully with the following message:

> node index.js

util.js:972
    throw new TypeError('The super constructor to "inherits" must not ' +
    ^

TypeError: The super constructor to "inherits" must not be null or undefined
    at Object.exports.inherits (util.js:972:11)
    at Object.<anonymous> (/Users/marvin/Documents/Code/zetta/node_modules/spdy/lib/spdy/connection.js:86:6)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/marvin/Documents/Code/zetta/node_modules/spdy/lib/spdy.js:20:19)

I am on macOS on node 7.7.3.

@kevinswiber
Copy link
Member

kevinswiber commented Apr 8, 2017

It looks like Zetta's broken on Node v7.x. The spdy module has a reference to process.EventEmitter, which has been deprecated as of v7.x. (See: https://github.com/spdy-http2/node-spdy/blob/3c7e45532a6083e1ae9a4fea26b70e62efa7eda4/lib/spdy/connection.js#L86).

The best option would be to upgrade Zetta's spdy module to the latest version, but last I heard, there were some issues with doing that.

(Edit: PS. If you manually edit that file to use require('events') instead of process.EventEmitter, you can get over the hump, locally, in the meantime. Not a long-term solution. Just a short-term band-aid with a dash of tomfoolery.)

@marvinroger
Copy link
Contributor Author

As of the v ^2.0.0 of sdpy indeed, require('events') is used. What's wrong with the latest version?

@kevinswiber
Copy link
Member

@AdamMagaluk You looked at this months ago, didn't you? Any notes from this?

@marvinroger
Copy link
Contributor Author

Quick question, why SPDY over HTTP/2?

@marvinroger
Copy link
Contributor Author

And, simple workaround:

process.EventEmitter = require('events').EventEmitter

const zetta = require('zetta')

zetta()
  .name('Test')
  .listen(1337, function () {
    console.log('Zetta is running')
  })

@AdamMagaluk
Copy link
Collaborator

@kevinswiber I looked at upgrading the SPDY client a while back and made some progress but never finished. There were a few of places in zetta that needed to change because of internal updates for the spdy client.

@marvinroger We choose SPDY originally because when zetta was first developed HTTP/2 was still in draft and not finalized. We've talked about upgrading. If we were to upgrade to the latest spdy client it would support moving to the http2 spec.

@kevinswiber
Copy link
Member

kevinswiber commented Apr 12, 2017

I'm actually taking a look at upgrading the spdy dependency now.

For a little history...

We started working on what would become Zetta in December of 2013. It wasn't until December of 2014 that the IETF working group would start work on HTTP/2. They used SPDY/4 as the base, and to my big surprise, HTTP/2 was a proposed standard by February of 2015. That's an insanely short amount of time for the IETF.

Of course, by December of 2014, our Z2Z protocol had already been built and working. It would still take many months after HTTP/2 became a proposed standard before there would be any working libraries or tooling. Looking back, I still think we made the right choice to stick with SPDY. We were already heavily invested and didn't have the cycles to spare. We can still look at upgrading down the road. We'd probably support HTTP/2 and SPDY/3.1 concurrently for backward compatibility.

@kevinswiber
Copy link
Member

FYI: Still working on this when I can. I'm making progress. I'm through the "WHY THE HELL WON'T THIS CONNECT?!" phase, and now I'm looking at a few other broken tests as a result of the upgrade.

@kevinswiber
Copy link
Member

Also, one benefit of upgrading the spdy dependency is that we can look to provide an HTTP/2 interface for the Client API when it's done.

@marvinroger
Copy link
Contributor Author

Great, thanks for the update. #349 will also need the spdy update so that ES2015 classes can be used. The current version does not support it.

@robalascott
Copy link

Thank you for simple work around ....

@svogl
Copy link

svogl commented Jul 27, 2017

... I ran into the same issue with node 8.2.1 , the hack solved it - thanks

@kevinswiber I put the 'process.EventEmitter = require('events').EventEmitter' line at the top of zetta.js which seems to work as well.
It's ugly but would save other users from falling over this - patch wanted?

@svogl
Copy link

svogl commented Jul 28, 2017

one more thing: when I link a zetta device on my machine running node 8.2.1 to a server with node 4.2.6 (on ubuntu xenial), I get a RangeError:

svogl@M:~/proj/zetta-liner$ Jul-28-2017 15:59:27 [server] Server (eyeLiner) eyeLiner listening on http://127.0.0.1:3338
Zetta is running at http://127.0.0.1:3338
Jul-28-2017 15:59:27 [peer-client] WebSocket to peer established (ws://akoya.local:3338/peers/eyeLiner)
zlib.js:201
      throw new RangeError('Invalid windowBits: ' + opts.windowBits);
      ^

RangeError: Invalid windowBits: 0
    at Inflate.Zlib (zlib.js:201:13)
    at new Inflate (zlib.js:520:8)
    at Object.Inflate (zlib.js:519:12)
    at Object.createInflate (/home/svogl/proj/zetta-liner/node_modules/spdy/lib/spdy/utils.js:46:22)
    at Pool.get (/home/svogl/proj/zetta-liner/node_modules/spdy/lib/spdy/zlib-pool.js:41:27)
    at Parser.onversion (/home/svogl/proj/zetta-liner/node_modules/spdy/lib/spdy/connection.js:107:19)
    at emitOne (events.js:115:13)
    at Parser.emit (events.js:210:7)
    at Parser.setVersion (/home/svogl/proj/zetta-liner/node_modules/spdy/lib/spdy/protocol/parser.js:208:8)
    at Parser.execute (/home/svogl/proj/zetta-liner/node_modules/spdy/lib/spdy/protocol/parser.js:229:12)

P.S. one machine is 32 bit (v8.2.1), the other 64..; I downgraded to v4.2.3 LTS release on the 32 bit machine and it seems to work now.

@wooliet
Copy link
Contributor

wooliet commented Aug 4, 2017

I also get RangeError with client running node 8.2.1 and hitting the Apigee Link instance of Zetta (not sure what version that's running but I imagine 0.12).

@AdamMagaluk
Copy link
Collaborator

@kevinswiber I was able to get a working example of our z2z protocol working with the latest spdy dependancy. See: https://github.com/AdamMagaluk/z2z-proxy Haven't tried making the changes in zetta and seeing what breaks yet...

Also looks like http/2 support has landed in Node.js master so could look at using that directly too, though not sure if all the apis will be available for the z2z use case. nodejs/node#14239

@hutualive
Copy link

@kevinswiber @AdamMagaluk any official patch or walk around ?
I'm on Node.JS 8.4.0 and upgrade to latest SPDY 3.4.7 but still the same error.

how to upgrade the SPDY dependencies to latest ? I'm confused with https://github.com/AdamMagaluk/z2z-proxy

how to apply it ?

thanks.

@AdamMagaluk
Copy link
Collaborator

@hutualive No not yet, the z2z-proxy just demonstrates that it would be possible to upgrade zetta to use the latest spdy library. It shows the number of small API changes needed, though I'm still working through one issue regarding spdy pings as the interface isn't exposed in the new version.

I plan to work on a zetta branch in the next week or two that would apply the updates. I'll keep this channel updated.

@khinze
Copy link

khinze commented Nov 27, 2017

Hi,

Is there any potential fix for this yet?

@khinze
Copy link

khinze commented Nov 27, 2017

RangeError: Invalid windowBits: 0
zlib.js:202
at Inflate.Zlib (zlib.js:202:13)
at new Inflate (zlib.js:546:8)
at Object.Inflate (zlib.js:545:12)
at Object.createInflate (c:\Repositories\node_modules\zetta\node_modules\spdy\lib\spdy\utils.js:46:22)
at Pool.get (c:\Repositories\rbg\iot-700-controller\node_modules\zetta\node_modules\spdy\lib\spdy\zlib-pool.js:41:27)
at Parser.onversion (c:\Repositories\r\node_modules\zetta\node_modules\spdy\lib\spdy\connection.js:107:19)

@AdamMagaluk
Copy link
Collaborator

@khinze I've been slowing working on a fix but unfortunately I haven't had much time to commit. Here is the latest "mostly" working version of zetta that fixes the issue. #360

@framlin
Copy link

framlin commented Jan 9, 2018

Hello,
could anyone please describe as litteral as possible, how this could be fixed?

What should I do, to upgrade spdy such that it would work again .. or that the modules I use, that use spdy work again?

thanks in advance

@AdamMagaluk
Copy link
Collaborator

@framlin Unfortunately there is no easy fix right now. Zetta and its spdy dependency do not work on node >= node v7.

The nodejs spdy module underwent a major rewrite in the later versions that we are now trying to use that broke a number of things in Zetta. I'm still working on getting it to work but it is not an easy task and unfortunately I do not have a lot of time to dedicate to it at the moment.

Long story short, use [email protected]. That's the best i can give you at the moment.

@ScottErholm
Copy link

At this point, spdy seems beyond deprecated -- mostly dead. Adam did a lot of work to get spdy 4 mostly working with zetta, but now even more bugs are popping up. How much effort is required to replace all references and hooks to spdy with node's native HTTP/2? Also, I believe there are functions relied upon that HTTP/2 can't provide, so some other modules would need to be added in place of spdy.

I don't believe anyone alone has the time/bandwidth to complete all the required work. I'd be willing to work on some chunks if one of the more knowledgeable architects could devise an upgrade path. zetta was a very nice platform, and I'd hate to see it die on the vine.

@AdamMagaluk
Copy link
Collaborator

Hi Scott,

I agree with you on spdy, it's been a nightmare and moving target trying to upgrade. I agree with your assessments on http/2 and i followed the Node.js API when it was being drafted and there were holes that zetta needed. But not sure what the state is I haven't been active in that for 2 years. Its worth re-evaluating.

During the SPDY upgrade i was also working under the constraint that Zetta versions be backwards compatible from a network and protocol perspective. If we're left with the option of making breaking changes and zetta being abandoned we could evaluate that contraint.

I'm always happy to advise and hop on a GVC and review PRs when I can. I agree with you and would love for Zetta to live on, I think it still has plenty of potential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants