search

libp2p / go-libp2p-circuit

Circuit Switching for libp2p
MIT License
48 stars 16 forks source link

fix: FullClose is now half-open #42

Closed dryajov closed 3 years ago

dryajov commented 6 years ago

It seems like FullClose needs to be called async. This is currently breaking https://github.com/ipfs/interop/pull/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?

Stebalien commented 6 years ago

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, https://github.com/ipfs/js-ipfs/issues/1445 is still a valid issue.

dryajov commented 6 years ago

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

Stebalien commented 6 years ago

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: https://github.com/libp2p/go-libp2p/pull/380

dryajov commented 6 years ago

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 commented 6 years ago

The issue ended up being on the js side after all, here is the PR addressing it - https://github.com/libp2p/js-libp2p-circuit/pull/28.

Stebalien commented 6 years ago

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 commented 3 years ago

With https://github.com/libp2p/go-libp2p-core/pull/166, we're going to be able to just call close and walk away.