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

change OpenStream to accept a context #29

Merged
merged 1 commit into from Dec 19, 2020
Merged

Conversation

marten-seemann
Copy link
Contributor

@willscott
Copy link
Contributor

should there be an issue for shutting down the stream when the context expires as a follow-on to this?

@Stebalien
Copy link
Member

should there be an issue for shutting down the stream when the context expires as a follow-on to this?

The context is just for opening the stream. This is actually how contexts are supposed to be used, we just tend to abuse them.

@willscott
Copy link
Contributor

should there be an issue for shutting down the stream when the context expires as a follow-on to this?

The context is just for opening the stream. This is actually how contexts are supposed to be used, we just tend to abuse them.

Am i correct in reading it that OpenStream does block on a network send of a window update indicating the new stream exists:
https://github.com/libp2p/go-yamux/blob/master/session.go#L201

If so, do we need to thread context down to the sending loop to be able to fail out of that blocking?

@marten-seemann
Copy link
Contributor Author

@willscott is right here in that we should not ignore the context entirely, as this PR (which just intends to satisfy the new interface) does. @Stebalien is right that the context should only apply to opening the stream, i.e. it applies only for the duration of OpenStream, everything that happens after OpenStream has returned doesn't matter.

I opened #30 to track this.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context deadline might need to percolate down to the actual network write over the transport, potentially through SetDeadline...

@marten-seemann marten-seemann marked this pull request as ready for review December 19, 2020 02:57
@marten-seemann marten-seemann merged commit 9285cf1 into master Dec 19, 2020
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
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.

None yet

4 participants