-
Notifications
You must be signed in to change notification settings - Fork 12
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
PutValue context not utilized or propagated into bootstrapping #28
Comments
Not so sure about moving bootstrap into pubsub, but I won't reject a good patch :) |
My thinking is that there should be a pubsub option that passes in a discovery service. If provided, pubsub would:
1 and 2 are difficult to do reliably outside pubsub. |
ok |
We might want to connect the bootstrap process with the routers as well, so that they have a say in whether more peers are needed. |
…messages in PubSub bootstrapping non-blocking
Any thoughts as to whether we should be maintaining/running a peer discovery service if we've
|
Ideally, we wouldn't block publishing to run publish. Should we provide a separate bootstrap method? Or something that returns a bootstrap handle (subset of subscribe functionality)? @vyzo how does gossipsub handle publishing on topics we're not subscribed to? |
@Stebalien running a blocking @vyzo please correct me if I'm wrong, but I think I can give a stab at this question. Regarding gossipsub and publishing topics we're not subscribed to,
It's worth noting the |
I kind of agree but we need to make sure to not block forever if, e.g., we're offline. |
@aschmahmann this is correct, we don't do anything special other than using the existing peers on publish.
We should have a timeout in the |
Given that operations like However, if we'd like to add some context passing into PubSub functions as well that seems reasonable. I'm just not convinced we should hard code any particular timeouts into PubSub itself. |
@aschmahmann this has been fixed, right? |
@Stebalien not really. We followed through on the suggestion to make the Content Routing/DHT Provide in the bootstrapping process non-blocking (see #29) which makes this issue much less painful. However, the rest of the bootstrapping operation still does not respect the My understanding was that instead of immediately tackling this by changing the issue in this repo that we would put together a PR to move the bootstrapping into the PubSub itself via the |
PubsubValueStore.PutValue
receives acontext.Context
but does not respect it. In particular, we have:go-libp2p-pubsub-router/pubsub.go
Line 92 in 2c8ac20
which uses the
PubsubValueStore
's context instead of thePutValue
context when calling the very expensivebootstrapPubsub
function.Additionally,
bootstrapPubsub
could be much less expensive if:go-libp2p-pubsub-router/pubsub.go
Line 345 in 2c8ac20
was moved into the go func right below it.
After speaking with @Stebalien my current plan is to start by moving the
Provide
into the go func, and then look into moving bootstrapping into PubSub via the Discovery (https://github.com/libp2p/go-libp2p-discovery) interface, as indicated by:go-libp2p-pubsub-router/pubsub.go
Line 338 in 2c8ac20
@vyzo @raulk Any thoughts or issues with this?
The text was updated successfully, but these errors were encountered: