Skip to content

Add context to client - #20

Closed
tzdybal wants to merge 1 commit into
mainfrom
tzdybal/p2p/context
Closed

Add context to client#20
tzdybal wants to merge 1 commit into
mainfrom
tzdybal/p2p/context

Conversation

@tzdybal

@tzdybaltzdybal commented Mar 10, 2021

Copy link
Copy Markdown
Contributor

To enable control over client life-cycle, we need a context. It's also passed to libp2p layer.

@liamsi
liamsi requested a review from WondertanMarch 18, 2021 09:53
Comment threadp2p/client.go
c.logger.Debug("seed", "addr", maddr.String())
// TODO(tzdybal): configuration param for connection timeout
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
ctx, cancel := context.WithTimeout(c.ctx, 3*time.Second)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we extract 3*time.Second to some documented const or rather var?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was thinking about configuration TBH, but for now I'll just use const.

@liamsiliamsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some nit, otherwise LGTM!

@tzdybal

Copy link
Copy Markdown
ContributorAuthor

I'll fix the nit in #17.

@tzdybal

Copy link
Copy Markdown
ContributorAuthor

Actually, I'll skip this one, as the changes are already included in #17.

@tzdybaltzdybal closed this Mar 18, 2021
@tzdybal
tzdybal deleted the tzdybal/p2p/context branch March 18, 2021 11:50
@tzdybal
tzdybal restored the tzdybal/p2p/context branch October 9, 2021 13:35
@tzdybal
tzdybal deleted the tzdybal/p2p/context branch October 9, 2021 13:37
@claudeclaudeBot mentioned this pull request Nov 5, 2025
11 tasks
Sign up for freeto 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

@tzdybal@liamsi