use correct context for dht notifs #1568
Conversation
License: MIT Signed-off-by: Jeromy <[email protected]>
| @@ -40,9 +40,12 @@ func (dht *IpfsDHT) GetClosestPeers(ctx context.Context, key key.Key) (<-chan pe | |||
| peerset.Add(p) | |||
| } | |||
|
|
|||
| // since the query doesnt actually pass our context down | |||
| // we have to hack this here. whyrusleeping isnt a huge fan of goprocess | |||
There was a problem hiding this comment.
since the query doesnt actually pass our context down
can you clarify what you mean? where / why doesnt it get passed down?
There was a problem hiding this comment.
so, the context gets passed into run, and then we create a goprocess. We set that goprocess to be closed when the context gets cancelled, and then we derive a context from that process to pass to the queryFuncs. it seems a little redundant.
There was a problem hiding this comment.
creating a process from a context, or a context from a process, is the same thing as deriving a new context from a parent context, or a process from a parent process. it looks a bit awkward because the models are a bit different, but it is the same idea. it's a process tree. it's just that the process can let you reason about teardown, whereas the context cant.
There was a problem hiding this comment.
(meaning that likely the same thing would happen with the contexts -- the derivation of child contexts. the reason for using process there (instead of a bare context) is about rate limiting and proper teardown).
There was a problem hiding this comment.
this still doesn't answer:
- where / why doesnt it get passed down?
There was a problem hiding this comment.
contexts have a Value field, which gives you a key value mapping that is tied to a particular chain of contexts. With the goprocess thing in the middle, those value requests dont get propagated up the chain like they would if we exclusively used contexts.
There was a problem hiding this comment.
ohhhh: the PublishQueryEvent thing needs ctx.Value. yeah that sucks
There was a problem hiding this comment.
yeah... i'm not sure of a 'right' way to do this. but its really nice to be able to use the context for that purpose, allows you to easily register for just the notifications that you are interested in
There was a problem hiding this comment.
maybe we should have it in process too? (or go back to always-having-a-context-in-a-process and allow:
v := px.Context().Value(k)
|
I'll merge this for now, but should probably fix goprocess to not kill the value trees. |
use correct context for dht notifs
I was trying to diagnose some dht issues i was noticing and i noticed that some entries were absent from the output of the query notifications. It looks like i was relying on the context to be passed through to the query functions. but at some point the goprocess stuff got shoved in the middle of it, so now we derive a context from a process that is derived from a context and pass that to the queryFunc. I put a little closure in that allows us to have the correct context within the queryFuncs to properly publish events.
License: MIT
Signed-off-by: Jeromy [email protected]