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

It should be possible to close a stream and then read from it. #18

Open
Stebalien opened this issue Sep 13, 2017 · 6 comments · May be fixed by #55
Open

It should be possible to close a stream and then read from it. #18

Stebalien opened this issue Sep 13, 2017 · 6 comments · May be fixed by #55
Labels
exp/wizard Extensive knowledge (implications, ramifications) required kind/bug A bug in existing code (including security flaws) status/ready Ready to be worked

Comments

@Stebalien
Copy link
Member

Putting this close call before the read causes the tests to fail. However, given that we're now allow half-closed streams, it should work.

It looks like something is seeing that the stream is closed and resetting the connection before responding.

@vyzo
Copy link
Contributor

vyzo commented Sep 13, 2017

So is this just a subtlety in the code?
Semantically, the close should be after the read, but perhaps we should add a comment there.

@Stebalien
Copy link
Member Author

IMO, the close should come after the last write. That is, close means "I'm done sending requests".

@whyrusleeping whyrusleeping added the status/ready Ready to be worked label Oct 17, 2017
@bigs bigs added kind/bug A bug in existing code (including security flaws) exp/wizard Extensive knowledge (implications, ramifications) required labels Sep 11, 2018
@Ichbinjoe
Copy link

This no longer seems to be the case as of now.

https://github.com/Ichbinjoe/go-libp2p-circuit/blob/preemptive-close/relay.go#L217

I made these changes and the tests appear to still pass. Whatever issue exists no longer seems to now.

@Stebalien
Copy link
Member Author

@Ichbinjoe mind making a PR? We'll need to make sure it doesn't break js-libp2p but it would be nice to get this fix in.

@Ichbinjoe Ichbinjoe linked a pull request Dec 27, 2018 that will close this issue
@Ichbinjoe
Copy link

I currently don't have the JS stack setup, but I'm sure over the next couple of days I can mess around with it more.

@jsign
Copy link

jsign commented Dec 3, 2019

While doing a toy-app for an article, I wanted to be somewhat strict with the documentation.
If I navigate to the documented semantics of close, we can find this.

// Close closes the stream for writing. Reading will still work (that is, the remote side can still write).

In the toy-app case, the side that opened the stream would only read stuff, so guided by that comment, I switched a defered Close(), to an immediate Close() (before reading the stream), which lead to a stream reset error.

Am I talking about the same thing discussed here? I'm running just go-libp2p@v0.4.2 (asking since this isn't that repo). If that's the case, maybe it would be better to delete that comment since it may mislead to a usage that doesn't behave as expected?

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) status/ready Ready to be worked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants