Skip to content
This repository was archived by the owner on Jul 21, 2023. It is now read-only.

fix: handling socket disconnect error gracefully #66

Closed
wants to merge 1 commit into from

Conversation

pgte
Copy link

@pgte pgte commented Jun 19, 2017

Not propagating some error messages that depict that the underlying resource has been closed.
Added an error message to the original Channel destroyed one.
Should fix ipfs/js-ipfs#883

@RichardLitt RichardLitt added the status/in-progress In progress label Jun 19, 2017
@coveralls
Copy link

coveralls commented Jun 19, 2017

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling 6b8043b on fix/handle-socket-error into 0425acc on master.

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.

If the channel is destroyed, the error must be propagated to every muxed stream so that the users of those can request another stream.

@pgte
Copy link
Author

pgte commented Jun 21, 2017

@diasdavid does it need to be an error and not simply the end of the stream?

@pgte
Copy link
Author

pgte commented Jun 21, 2017

@diasdavid with this change, and at the js-ipfs level, instead of an uncaughtException (which I could not find the culprit of), the stream ends correctly and the peer leaves gracefully.

@daviddias
Copy link
Member

Yeah, it needs to be an error that gets emitted in every muxed stream so that each protocol understand that there was an error in transmission.

It seems that one of those errors is not getting caught somewhere in the code and gets thrown to the process.

@pgte
Copy link
Author

pgte commented Jun 21, 2017

@diasdavid I guess to makes sense if the protocols presume that the message was delivered if there was no error... In this case, doesn't the Channel destroyed error that is currently being masked also a problem?

@daviddias
Copy link
Member

For the sake of the 'protocol' (i.e bitswap, dht, etc) own operations, it just really needs to know that its connection was dropped because of a) was graciously closed, b) some error caused the connection to crash.

We don't propagate specific errors from each transport up.

Makes sense? Not 100% sure that we are in the same page :)

@pgte
Copy link
Author

pgte commented Jun 21, 2017

@diasdavid nevermind. I was being guided by this piece of code and this comment, but I admit it looks a bit hacky.

Nevermind this for now then, I'll keep investigating where this error should have been handled.

@pgte pgte closed this Jun 27, 2017
@RichardLitt RichardLitt removed the status/in-progress In progress label Jun 27, 2017
@daviddias daviddias deleted the fix/handle-socket-error branch June 27, 2017 21:39
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.

Uncaught Error: underlying socket has been closed
4 participants