Skip to content
This repository was archived by the owner on Oct 19, 2022. It is now read-only.

fix: catch uncaught exceptions from pull in dialer.handle #44

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Aug 14, 2018

This should prevent ipfs/js-ipfs#1446 from crashing ipfs nodes. Uncaught errors like Already piped will be caught and logged as errors.

@jacobheun jacobheun requested a review from alanshaw August 14, 2018 18:39
@jacobheun
Copy link
Contributor Author

I'm still not sure what the root cause here is. My thought is that it is likely do to flapping connections, since the errors seem to come in waves when they happen. The dialer is trying to perform the multistream handshake and attempts to read while it's getting hung up on. I can look at getting some test suites added around this later, but this should get people unblocked from crashing nodes.

@jacobheun jacobheun requested a review from daviddias August 14, 2018 19:02
@jacobheun
Copy link
Contributor Author

With debugging turned on locally I was able to get it to trigger:

Browser

mss:dialer	:error (1nu9ou) Error: already piped    at sink
(http://localhost:3006/static/js/bundle.js:229103:22)    at consume
(http://localhost:3006/static/js/bundle.js:228549:18)    at consume
(http://localhost:3006/static/js/bundle.js:228549:18)    at Connection.consume
(http://localhost:3006/static/js/bundle.js:228549:18)    at pull
(http://localhost:3006/static/js/bundle.js:4790:9)    at Dialer.handle
(http://localhost:3006/static/js/bundle.js:7119:7)    at handleSafe
(http://localhost:3006/static/js/bundle.js:122515:12)    at Dialer._attemptMuxerUpgrade
(http://localhost:3006/static/js/bundle.js:122750:5)    at Dialer._createMuxedConnection
(http://localhost:3006/static/js/bundle.js:122713:10)    at waterfall
(http://localhost:3006/static/js/bundle.js:122601:12)    at nextTask
(http://localhost:3006/static/js/bundle.js:19611:10)    at next
(http://localhost:3006/static/js/bundle.js:19621:5)    at
http://localhost:3006/static/js/bundle.js:18157:12    at Dialer._createBaseConnection
(http://localhost:3006/static/js/bundle.js:122646:14)    at cb
(http://localhost:3006/static/js/bundle.js:122598:12)    at nextTask
(http://localhost:3006/static/js/bundle.js:19611:10)    at
./node_modules/async/waterfall.js.exports.default (http://localhost:3006/static/js/bundle.js:19624:3)   
at Dialer._establishConnection (http://localhost:3006/static/js/bundle.js:122596:5)    at cb
(http://localhost:3006/static/js/bundle.js:122558:12)    at nextTask
(http://localhost:3006/static/js/bundle.js:19611:10)    at
./node_modules/async/waterfall.js.exports.default (http://localhost:3006/static/js/bundle.js:19624:3)   
at Dialer.dial (http://localhost:3006/static/js/bundle.js:122557:5)    at Switch.dial
(http://localhost:3006/static/js/bundle.js:122951:19)    at _getPeerInfo (http://localhost:3006/static/js/bundle.js:130259:20)    at setImmediate (http://localhost:3006/static/js/bundle.js:129952:24)    at http://localhost:3006/static/js/bundle.js:18539:10    at run (http://localhost:3006/static/js/bundle.js:266430:9)    at runIfPresent (http://localhost:3006/static/js/bundle.js:266465:11)    at onGlobalMessage (http://localhost:3006/static/js/bundle.js:266507:9)

Node

 mss:dialer	:error (dnp5yf) Error: already piped +0ms

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Clearly not the root of the problem but until it can be solved the code change LGTM and will hopefully prevent some daemon crashes.

)
} catch (err) {
this.log.error(err)
callback(err)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this callback be called twice this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The callback is already wrapped by once to protect against this.

Additionally the handshake would need to have already succeeded or errored, instead of throwing, for the callback to have been called. I don't believe it will happen, but either way once should protect against the case that it is called multiple times.

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM

@jacobheun jacobheun merged commit d3cd2e5 into master Aug 15, 2018
@jacobheun jacobheun deleted the fix/already-piped branch August 15, 2018 11:09
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.

3 participants