Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Close streams when done with them #1445

Closed
Stebalien opened this issue Jul 16, 2018 · 5 comments
Closed

Close streams when done with them #1445

Stebalien opened this issue Jul 16, 2018 · 5 comments
Assignees
Labels
exp/wizard Extensive knowledge (implications, ramifications) required kind/bug A bug in existing code (including security flaws) kind/not-helia-issue P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/libp2p Topic libp2p

Comments

@Stebalien
Copy link
Member

Our stream muxers all support half-open connections. Therefore, if you receive an EOF on a stream, you must send an EOF on that stream to fully close it. Otherwise, the other end will (if correctly implemented) sit around waiting for the EOF.

I know that the part of bitswap that accepts blocks from peers isn't doing this and I wouldn't be surprised if this doesn't happen in other places as well (stream closing has been a persistent problem in go-ipfs as well).

This showed up in ipfs/interop#28.

@alanshaw alanshaw added kind/bug A bug in existing code (including security flaws) exp/wizard Extensive knowledge (implications, ramifications) required status/ready Ready to be worked P1 High: Likely tackled by core team if no one steps up labels Jul 17, 2018
@Stebalien
Copy link
Member Author

This should probably be difficulty:easy. js-ipfs just needs to close the stream when done.

@daviddias
Copy link
Member

@Stebalien I'm pretty sure we always close when done. Couldn't find any location where that is not happening.

@Stebalien
Copy link
Member Author

You sure you don't need something like:

diff --git a/src/network.js b/src/network.js
index 0e914c5..ae91bfa 100644
--- a/src/network.js
+++ b/src/network.js
@@ -81,6 +81,7 @@ class Network {
         if (err) {
           this.bitswap._receiveError(err)
         }
+        pull(pull.empty(), conn)
       })
     )
   }

We have a duplex stream so we can't just read until the end, we have to tell the other side that we aren't sending anything (unless pull does this under the covers...).

@SgtPooki
Copy link
Member

js-ipfs is being deprecated in favor of Helia. You can #4336 and read the migration guide.

Please feel to reopen with any comments by 2023-06-02. We will do a final pass on reopened issues afterwards (see #4336).

@achingbrain are we doing this properly in libp2p (with test)? do we need to migrate this to libp2p?

@achingbrain
Copy link
Member

I'm not sure that this is an issue, you can watch the number of open streams on a given connection with Graphana and @libp2p/interface-metrics and it doesn't grow over time.

I'm going to close this but it can be reopened against ipfs-bitswap if anything turns up.

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 kind/bug A bug in existing code (including security flaws) kind/not-helia-issue P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/libp2p Topic libp2p
Projects
No open projects
Status: Done
Development

No branches or pull requests

5 participants