Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

fix: FullClose is now half-open #42

Closed
wants to merge 1 commit into from

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Jul 31, 2018

It seems like FullClose needs to be called async. This is currently breaking ipfs/interop#27. There are other instances of FullClose that are not being executed in a go routine as well, and I wonder if those should also be fixed as well?

@ghost ghost assigned dryajov Jul 31, 2018
@ghost ghost added the status/in-progress In progress label Jul 31, 2018
@daviddias daviddias requested a review from Stebalien July 31, 2018 13:28
@Stebalien
Copy link
Member

This one is working as intended (actually, we should be calling Close after the write and then reading until the end of the stream but that's even more likely to break compatibility with older nodes.

JavaScript needs to close it's streams. The issue in bitswap was that we were blocking other bitswap peers. However, ipfs/js-ipfs#1445 is still a valid issue.

@dryajov
Copy link
Member Author

dryajov commented Jul 31, 2018

👍 on close the js streams, but this still possibly needs to be ran in a go routine instead of blocking?

@Stebalien
Copy link
Member

I intentionally didn't do that to avoid creating additional goroutines. However, if we are just going to close this and throw away the result, I'd almost just reset the stream. Same question as: libp2p/go-libp2p#380

@dryajov
Copy link
Member Author

dryajov commented Jul 31, 2018

There might be something else going on, the stream should be getting closed on the js side (I'm doing it explicitly in my tests now, but even letting the stream reach EOF should obviously work) but it still no dice. Will dig deeper and report back.

@dryajov
Copy link
Member Author

dryajov commented Jul 31, 2018

The issue ended up being on the js side after all, here is the PR addressing it - libp2p/js-libp2p-circuit#28.

@dryajov dryajov closed this Jul 31, 2018
@ghost ghost removed the status/in-progress In progress label Jul 31, 2018
@Stebalien
Copy link
Member

Actually, let's leave this open. The way this currently is, we'll end up adding a round-trip where we probably shouldn't.

@Stebalien Stebalien reopened this Jul 31, 2018
@ghost ghost assigned Stebalien Jul 31, 2018
@ghost ghost added the status/in-progress In progress label Jul 31, 2018
@Stebalien Stebalien added status/deferred Conscious decision to pause or backlog and removed status/in-progress In progress labels Aug 11, 2018
@bigs bigs added optimization exp/wizard Extensive knowledge (implications, ramifications) required labels Sep 11, 2018
@Stebalien
Copy link
Member

With libp2p/go-libp2p-core#166, we're going to be able to just call close and walk away.

@Stebalien Stebalien closed this Oct 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/wizard Extensive knowledge (implications, ramifications) required optimization status/deferred Conscious decision to pause or backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants