Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add context to DialPeer interface #272

Merged
merged 8 commits into from
Nov 5, 2014
Merged

add context to DialPeer interface #272

merged 8 commits into from
Nov 5, 2014

Conversation

btc
Copy link
Contributor

@btc btc commented Nov 5, 2014

@whyrusleeping @jbenet CR

I added context.Context to the DialPeer(...) interface method to preserve bitswap's API contract (that bitswap.GetBlock will return when the parent cancels its context). To make this possible, I updated signatures throughout the net and routing packages.

Down in the swarm, discovered that Swarm.Dial ignores the caller's context and instead passes it's own into the net.conn.Dialer. Since the function is being called in the context of the caller, the correct behavior is for swarm to pass the caller's context. Made this change in 7a2805e.

@jbenet Are there any side effects to worry about? yes

I was excited to follow these calls down the stack. The net layer is in pretty good shape, and will continue to improve as the system matures and we discover useful conventions for large-scale concurrent design.

@jbenet
Copy link
Member

jbenet commented Nov 5, 2014

Guessing the previous comments are no invalidated?

@maybebtc Do LGTM!!

@btc
Copy link
Contributor Author

btc commented Nov 5, 2014

It's still important to pass the caller's context down to the leaves. Just infeasible at the moment.

to merge after #271

err := util.Do(ctx, func() error {
_, err := n.swarm.Dial(p)
return err
})
Copy link
Member

Choose a reason for hiding this comment

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

this is so nice.

@jbenet
Copy link
Member

jbenet commented Nov 5, 2014

@maybebtc other than name LGTM

@btc
Copy link
Contributor Author

btc commented Nov 5, 2014

fixed name

btc pushed a commit that referenced this pull request Nov 5, 2014
add context to DialPeer interface
@btc btc merged commit d742984 into master Nov 5, 2014
@btc btc removed the status/in-progress In progress label Nov 5, 2014
@btc btc deleted the context-dial-peer branch November 5, 2014 18:08
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants