Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

[WIP] Move to pull-streams #16

Merged
merged 5 commits into from
Sep 6, 2016
Merged

[WIP] Move to pull-streams #16

merged 5 commits into from
Sep 6, 2016

Conversation

dignifiedquire
Copy link
Member

No description provided.

@@ -2,7 +2,7 @@
"name": "libp2p-spdy",
"version": "0.8.1",
"description": "SPDY 3.1 implementation wrapper that is compatible with libp2p Stream Muxer expected interface",
"main": "lib/index.js",
"main": "src/index.js",
Copy link
Member

@daviddias daviddias Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • change before merge

@daviddias
Copy link
Member

daviddias commented Aug 18, 2016

Have you run the https://github.com/libp2p/interface-stream-muxer tests? I would love to see a time benchmark between using regular streams and pull streams

Let's also have tests for:

  • destroy underlying pull stream propagates the close and error to all the streams above
const socket = tcp.connect
// do the spdy thing, open a bunch of streams (leave them open)
socket.destroy()
// asses that all the streams, including the libp2p-spdy instance emits an error
  • close one of the muxed streams doesn't close others that are open
a) open 5 muxed streams,
b) close one, make sure the others don't close
  • close on spdy doesn't close until all the streams that are being muxed are closed too
a) open 5 muxed streams
b) call close/end on the spdy instance, make sure it waits for all of the muxed streams to finish

@dignifiedquire
Copy link
Member Author

Have you run the https://github.com/libp2p/interface-stream-muxer tests? I would love to see a time benchmark between using regular streams and pull streams

I haven't run them yet, I will go and update them + move them to mocha as that way they will be always run with the main test suite.

@daviddias
Copy link
Member

they will be always run with the main test suite.

They can be run on release, but not every single as they take some quite of time to finish.

@daviddias
Copy link
Member

Getting consistent:

messages 1446
endStreams 14
streamsCreated 100

on the:

    it.only('100 stream with 100 msg', (done) => {
      spawn(muxer, 100, 100, done)
    })

@daviddias
Copy link
Member

daviddias commented Aug 26, 2016

More detailed:

  # nStreams: 1 nMsg 1
streamsCreated 1
endStreams 1
collectStreams 1
msgSent 1
msgReceived 1

 # nStreams: 1 nMsg 10
streamsCreated 1
endStreams 1
collectStreams 1
msgSent 10
msgReceived 10

 # nStreams: 1 nMsg 100
streamsCreated 1
endStreams 1
collectStreams 1
msgSent 100
msgReceived 100

 # nStreams: 10 nMsg 1
streamsCreated 10
endStreams 10
collectStreams 10
msgSent 10
msgReceived 10

 # nStreams: 10 nMsg 10
streamsCreated 10
endStreams 10
collectStreams 10
msgSent 100
msgReceived 100

 # nStreams: 10 nMsg 100
streamsCreated 10
endStreams 10
collectStreams 10
msgSent 1000
msgReceived 1000

 # nStreams: 100 nMsg 1
streamsCreated 100
endStreams 100
collectStreams 100
msgSent 100
msgReceived 100

 # nStreams: 100 nMsg 10
streamsCreated 100
endStreams 100
collectStreams 100
msgSent 1000
msgReceived 1000

 # nStreams: 100 nMsg 100
streamsCreated 100
endStreams 14
collectStreams 14
msgSent 10000
msgReceived 1446

@daviddias
Copy link
Member

@dignifiedquire could you push an update to this branch with the remove of the throttle on stress tests and the pointer to the forked version of spdy?

@dignifiedquire
Copy link
Member Author

@diasdavid done

@daviddias
Copy link
Member

@dignifiedquire still missing these tests:

#16 (comment)

@dignifiedquire
Copy link
Member Author

Have you run the https://github.com/libp2p/interface-stream-muxer tests? I would love to see a time benchmark between using regular streams and pull streams

It's not using the same test system anymore, but here are the before and after times

# current master
total:     130127
passing:   130127
duration:  11.4s
npm run compliance  14.52s user 1.42s system 131% cpu 12.108 total
# pull
npm run test:node  5.70s user 0.55s system 107% cpu 5.824 total

@dignifiedquire
Copy link
Member Author

@diasdavid could you write some pseudo code for the tests you are suggesting, I'm not entirely sure what you want.

@daviddias
Copy link
Member

@dignifiedquire added more info there - #16 (comment)

tcp = new Tcp()
})

it('attach to a websocket, as listener', (done) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be to a tcp socket

@daviddias
Copy link
Member

daviddias commented Sep 6, 2016

Last things for the merge

@dignifiedquire
Copy link
Member Author

dignifiedquire commented Sep 6, 2016

close on spdy doesn't close until all the streams that are being muxed are closed too

test exists, but fails, not sure why though :(

  • 4 out of 5 streams are closed
  • the last one is closed on the listener side, but not on the dialer side
  • all data is received and sent
  • spdy-transport does not emit the close event on the last one on the raw node.js stream

@daviddias
Copy link
Member

daviddias commented Sep 6, 2016

@dignifiedquire you mentioned that spdy is build to destroy all the muxedConns if we call it to end. If that is the case, then it should close all the streams and not wait for them to finish.

what is the raw Node.js stream? the one spdy is mounted on top?

One more thing:

  • let's add to the list the same test, but make it happen where data is still being sent.

@dignifiedquire
Copy link
Member Author

you mentioned that spdy is build to destroy all the muxedConns if we call it to end

This was a misunderstanding from my side. Calling .end will

  • Send goaway frame
  • Error all newly created streams with New stream after goaway
  • Attach an optional close handler, which is triggered once all open streams are closed

@daviddias
Copy link
Member

awesome, so it doesn't abruptly all flowing streams, that is what I thought :)

@dignifiedquire
Copy link
Member Author

let's add to the list the same test, but make it happen where data is still being sent.

that's what the test currently does, send data on 5 streams delayed, and trigger .end before they are finished

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants