-
Notifications
You must be signed in to change notification settings - Fork 37
fix: prevent undefined error during a mutual hangup #260
Conversation
src/index.js
Outdated
@@ -110,6 +110,11 @@ class Switch extends EE { | |||
this.stats.stop() | |||
series([ | |||
(cb) => each(this.muxedConns, (conn, cb) => { | |||
// If the connection was destroyed while we are hanging up, continue | |||
if (conn === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check be so restrictive? Perhaps an if (conn) {...}
should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had a more generic check if (!conn)
, but changed it to undefined as there shouldn't be a situation where conn is something other than undefined or an object with the muxer property. The only way it would be something other than those two options is if deletions of the muxers changes internally, or if they are overridden by another library.
I suppose the tiny performance benefit during a swarm stop isn't worth the longer term resilience of the generic check. I can update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update pushed, this should be good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, other than the comment I left.
Pull Request Test Coverage Report for Build 867
💛 - Coveralls |
This resolves an issue where if connected nodes hang up at the same time, such as during testing, a muxedConnection could be deleted from the switch at the same time as it's async iteration was occurring to end those connections. This would result in an undefined error.
This PR ensures we don't attempt to close connections that have been hung up on.
You can see the passing CI build for js-libp2p that uses this branch here: https://ci.ipfs.team/blue/organizations/jenkins/libp2p%2Fjs-libp2p/detail/chore%2Fupdate-switch/4/pipeline
connects to libp2p/js-libp2p#198