-
Notifications
You must be signed in to change notification settings - Fork 178
Add findprovs #47
base: master
Are you sure you want to change the base?
Add findprovs #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments, otherwise LGTM
shell.go
Outdated
return nil, resp.Error | ||
} | ||
|
||
outchan := make(chan pstore.PeerInfo, 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make 4 a constant (with a comment), or add comment on why 4
|
||
go func() { | ||
<-ctx.Done() | ||
resp.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see why not defer resp.Close() in the previous go-routine but I assume there is a reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, json.NewDecoder(resp.Output)
will hang until the resp is closed. So this thing breaks out the NewDecoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, if NewDecoder
hangs, then it's going to hang until someone cancels the context, because resp.Close()
is not called until then, and the context won't be cancelled because NewDecoder
is hanging ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We self cancel/exit when responses from the endpoint end. If someone wants to exit early he has to cancel context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not NewDecoder but decoder.Decode
.
I'm a bit afraid of the added dependencies -- let's revisit this when we have the first parts of the Core API implemented here. |
@lgierth agreed |
Hi! Any update on this? Currently, how can I use |
No description provided.