Testing libp2p-spdy on the browser #6

Merged
merged 5 commits into from Apr 14, 2016

Projects

None yet

4 participants

@xicombd
Member
xicombd commented Mar 22, 2016

Re-opening #5, from a branch inside this repo.

Quoting @diasdavid:

We've investigated and currently something weird happens where spdy streams inside the browser work if they are handshaked over stream-pairs, however when connecting a browser - node.js over websockets, opening a stream from the browser succeeds while opening a stream from Node.js fails. This could indicate a problem with the underlying transport, however, our tests with https://github.com/diasdavid/js-libp2p-multiplex and libp2p-websockets work flawlessley.

  • browser - browser
  • browser - node (fails on node opening a stream)
xicombd and others added some commits Mar 21, 2016
@xicombd xicombd Add browser tests (failing)
8efaa12
@diasdavid diasdavid clean up
44f7cf3
@xicombd xicombd Add spdy over ws tests
6665806
@diasdavid diasdavid commented on an outdated diff Mar 22, 2016
tests/spdy-over-ws-test.js
+ })
+ conn.pipe(bl((err, data) => {
+ expect(err).to.not.exist
+ expect(data.toString()).to.equal('heyho')
+ done()
+ }))
+ })
+
+ it('open a multiplex stream from listener and write to it', (done) => {
+ dialer.once('stream', (conn) => {
+ conn.pipe(conn)
+ })
+
+ const conn = listener.newStream()
+ conn.write('hey')
+ conn.write('ho')
@diasdavid
diasdavid Mar 22, 2016 Member

@xicombd this is not enough, they just get buffered and written on the same packet. It needs to be more stress tested

@diasdavid diasdavid one extra stress test
12810d9
@diasdavid
Member

Ok, state of things:

We have a bug when opening a stream from the Server to the Browser in the spdy multiplex connection established through libp2p-websockets:

Uncaught Error: need dictionary:  (/Users/david/code/protocol-labs/_libp2p/js/js-libp2p-spdy/tests/browser-nodejs/browser.js:3061)
Error: Uncaught Error: need dictionary:  (/Users/david/code/protocol-labs/_libp2p/js/js-libp2p-spdy/tests/browser-nodejs/browser.js:3061)

So far, we've ruled out

  • libp2p-websockets is not broken. We have run several tests on it and we've used libp2p-multiplex, another stream muxer, to run the same tests that we are doing with spdy and everything works like a charm
  • libp2p-spdy works in the browser. We've run tests running spdy on top of a stream-pair inside the browser and everything goes fine.
  • libp2p-spdy works on top of libp2p-websockets, if ran on Node.js to Node.js

The only thing that causes that error is really opening a libp2p-websockets connection from browser to a node.js process, and then opening a multiplexed stream from node.js. Very important to note that if we open a multiplex spdy stream from the browser to node.js, it works well.

@indutny, can I get some of your attention to this issue, I've tried to narrow down the scope of the issue as most as I could and, although I cannot give 100% guarantee that the problem is with spdy-transport, the error keeps coming from spdy-transport.

HALP 😱

@dignifiedquire
Member

Started debugging the issue. My first finding is that the browser receives a message that is not inflatable by zlib.inflate. The message is

var buf = new Buffer([56, 48, 227, 198, 167, 194, 0, 103, 0, 152, 255, 0, 0, 0, 5, 0, 0, 0, 7, 58, 109, 101, 116, 104, 111, 100, 0, 0, 0, 4, 80, 79, 83, 84, 0, 0, 0, 8, 58, 118, 101, 114, 115, 105, 111, 110])
buf.toString('hex') // => "3830e3c6a7c200670098ff00000005000000073a6d6574686f6400000004504f5354000000083a76657273696f6e"
buf.toString() // => "80�Ƨ��g�����������:method����POST����:version"

var zlib = require('zlib')
zlib.inflate(buf) 
// => Error: Missing dictionary
//    at Zlib._handle.onerror (zlib.js:363:17)
@dignifiedquire
Member

Some more progress, inflating with the correct dictionary from spdy-transport results in a successful inflation. So it looks like the zlib in the browser is not using the correct dictionary. This is the dictionary: https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/protocol/spdy/dictionary.js#L205

@dignifiedquire
Member

So it turns out it's actually documented why this does not work. In the browser zlib is replaced with browserify-zlib, which states in its readme that the dictionary option is not implemented. So node supports the dictionary and the browser doesn't, if browser <-> browser communication is tested they both ignore it and it works, but if you start sending data from node to the browser, the browser is unable to inflate resulting in this message as it's not able to handle the dictionary.

@diasdavid
Member

14:04 daviddias: well this is the library you need to patch in the first place to get that done: https://github.com/nodeca/pako
14:05 daviddias: nodeca/pako#69 (comment) it was only due to time constraints they didn't add it, so my guess is they'd be happy to accept a PR

SPDY uses zlib to compress its blocks, in order to get spdy working between browser and node.js (and other implementations), we need:

Need to implement in the JS impl:

Tests from Node.js that should pass:

@indutny
indutny commented Mar 24, 2016

Looks like you figured it out, guys!

@indutny
indutny commented Mar 24, 2016

Why not use http2 in browser, though?

@diasdavid
Member

thank you anyways, @indutny :D

We could and want to use http2 too, still have to create support for that in the go implementation

@dignifiedquire
Member

First implementation for pako is up here: nodeca/pako#77

cc @diasdavid

@dignifiedquire dignifiedquire referenced this pull request in ipfs/newsletter Mar 29, 2016
Closed

Collection for IPFS Weekly #9: March 21-29 #31

@diasdavid
Member

@dignifiedquire I saw this branch with the new browserify-zlib (ipfs/browserify-zlib#1) running in your machine. Seems you never had the chance to push the commit. Could you do it now?

@dignifiedquire
Member

@diasdavid there were no changes, just installing the correct browserify-zlib does the trick

@diasdavid diasdavid it all works now - refactored tests, let it be greeeeen
0983f11
@diasdavid
Member

greeen finally :D

@diasdavid diasdavid merged commit cabae63 into master Apr 14, 2016

2 of 3 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@diasdavid diasdavid deleted the browser-tests branch Apr 14, 2016
@RichardLitt RichardLitt referenced this pull request in ipfs/ipfs Apr 20, 2016
Open

IPFS Weekly Updates #151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment