[WIP] feat: handle stream muxer errors #245
Conversation
improve circuit relay error msgs
ghost
assigned 
dryajov
changed the title
|
Requires - libp2p/js-libp2p-mplex#75 |
daviddias
requested review from
dignifiedquire,
pgte and
daviddias
and removed request for
dignifiedquire
test/stream-muxers.node.js
Outdated
| @@ -22,14 +22,15 @@ const Switch = require('../src') | |||
| describe('Stream Multiplexing', () => { | |||
| [ | |||
| multiplex, | |||
| spdy | |||
| spdy // TODO: do we still support this? | |||
There was a problem hiding this comment.
Of course we still support. The whole design of libp2p is to support multiple building blocks.
There was a problem hiding this comment.
spdy doesn't return any errors when the stream abruptly terminates. We should make the behaviour consistent then.
What should happen - error on abrupt termination or silent skip?
test/stream-muxers.node.js
Outdated
| it('should fail graciously on dead stream mux', function (done) { | ||
| if (sm.multicodec === '/spdy/3.1.0') { | ||
| this.skip() | ||
| } |
There was a problem hiding this comment.
Skipping because the behaviour is not consistent across muxers - spdy doesn't return any errors on abrupt termination.
src/dial.js
Outdated
| } | ||
|
|
||
| cb(null, conn) | ||
| }) |
There was a problem hiding this comment.
why not just newStream(cb)?
src/dial.js
Outdated
| @@ -208,7 +213,13 @@ function dial (swtch) { | |||
| } | |||
|
|
|||
| function openConnInMuxedConn (muxer, cb) { | |||
| cb(muxer.newStream()) | |||
| muxer.newStream((err, conn) => { | |||
There was a problem hiding this comment.
This asking to change the Stream Muxer interface. https://github.com/libp2p/interface-stream-muxer needs to be updated.
There was a problem hiding this comment.
I didn't really change newStream interface, it already takes a callback as it is. I reverted this to return a connection as before. This PR just tries to propagate the error if it's returned by the multiplexer - before it would just be ignored.
|
I'm still not sure if we need this at all. This change was driven by -libp2p/js-libp2p-mplex#75, which would simply So the question remains - should we handle the errors in |
Pull Request Test Coverage Report for Build #6
💛 - Coveralls |
|
@pgte Any update on this thread? When can I expect this issue to be fixed. If anyone can direct me what needs to be done. I can hop on fixing this. |
|
@harrshasri I'm not the one assigned to this. You should probably be speaking to @dryajov :) |
|
@pgte Aah ... Okay I did tag you just because you were requested for reviewing this. |
|
Thanks for bringing this up @harrshasri, this issue needs some clarification for sure! The current state is as follows:
Also libp2p/js-libp2p-mplex#75 might not be required anymore since libp2p/js-libp2p-mplex#76 is almost here, but untill then the above dependencies need to be resolved. |
|
not relevant anymore |
I'm not sure why we weren't handling stream muxer errors here at all, but if there is a good reason and the error handling changes are not needed we can revert them. The really important change is in libp2p-mplex (libp2p/js-libp2p-mplex#75) that prevents the throw from crashing the entire node.