-
Notifications
You must be signed in to change notification settings - Fork 19
fix: transport should not handle connection if upgradeInbound throws #32
fix: transport should not handle connection if upgradeInbound throws #32
Conversation
1322752
to
aada16d
Compare
5348933
to
faf35c8
Compare
@jacobheun I tried to do a dynamic import for the |
The segfault is happening after the tests have finished. I haven't been able to determine where it's coming from yet, but I can look into this more later this week. |
yes, I think that it is the |
9a62650
to
71a30b6
Compare
After a small research, It's seem to crash with all node versions > 10 node-webrtc/node-webrtc#636 I added a With this, I will unblock all the outdated deps and open PRs |
@@ -40,7 +41,21 @@ module.exports = ({ handler, upgrader }, options = {}) => { | |||
const maConn = toConnection(channel, listener.__connections) | |||
log('new inbound connection %s', maConn.remoteAddr) | |||
|
|||
const conn = await upgrader.upgradeInbound(maConn) | |||
channel.on('signal', (signal) => { |
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.
This listener doesn't seem to be getting cleaned up anywhere. We need to make sure we're doing cleanup on the channel. I believe there is some code in the dialer that we should probably abstract out and reuse.
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 added the remove listeners.
Regarding abstracting the dialer, I think the differences are significant enough to add complexity with abstraction. I tried some changes abstracting them, but I think it made things even more confusing. These event handlers with callbacks are always not clean 😖 I think I can change the dialer to removeAllListeners instead of providing the actual callback. With that, we can abstract a clean function that cleans all the listeners, but not sure if it is worth
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
Fixed per discussion on libp2p/js-libp2p#523#discussion_r359997872