miscellaneous +/- (commands, dht, net) #395
Conversation
btc
force-pushed
the
misc/2014-11-11th-hour
branch
2 times, most recently
from
cda4f1e to
dec6dad
Compare
|
Let's freeze this PR here (904e9d5) before it gets too big. Would appreciate CR. @jbenet if this is too large, let me know. I can split it apart into 2 or 3 smaller PRs if necessary. And if you're interested, the routing changes have also been cherry-picked here: https://github.com/jbenet/go-ipfs/compare/fix/routing-399%2C400%2Cetc?expand=1 |
| @@ -262,6 +262,16 @@ func (c *MultiConn) ID() string { | |||
| return string(ids) | |||
| } | |||
|
|
|||
| func (c *MultiConn) Conns() []Conn { | |||
There was a problem hiding this comment.
Does this actually solve the problem? What if a connection is added or remove after the conns call completes and before the loop finishes?
There was a problem hiding this comment.
I dont think that's what the test is meant to assert. The test is asserting that once Close has been executed on all underlying conns, the MultiConn is Closed.
https://github.com/jbenet/go-ipfs/blob/misc/2014-11-11th-hour/net/conn/multiconn_test.go
Because of this issue, though MC probably shouldn't expose it's underlying Conns. I'm not sure what the design goals are for MC. @jbenet's perspective?
There was a problem hiding this comment.
Because of this issue, though MC probably shouldn't expose it's underlying Conns. I'm not sure what the design goals are for MC. @jbenet's perspective?
MC doesn't need to expose underlying Conns yet. There may be a need in the future but not here now. either way is fine.
There was a problem hiding this comment.
My main concern is addressing the race condition that occasionally breaks the build:
Is there another way you'd prefer that doesn't expose the conns?
func TestMulticonnClose(t *testing.T) {
// t.Skip("fooo")
log.Info("TestMulticonnSendUnderlying")
ctx := context.Background()
c1, c2 := setupMultiConns(t, ctx)
for _, c := range c1.conns { // before, race
c.Close()
}
for _, c := range c2.Conns() { // after, safe
c.Close()
}https://github.com/jbenet/go-ipfs/blob/misc/2014-11-11th-hour/net/conn/multiconn_test.go#L303
There was a problem hiding this comment.
maybe same thing you're doing, just unexpose the method? but either way works, really. nbd.
There was a problem hiding this comment.
yeah i agree it's nbd. just want to get go test -race to pass.
- privatize e33e89a
|
A few comments, but otherwise LGTM! Lots of bugfixes it seems. |
https://build.protocol-dev.com/job/go-ipfs.test.go.race.nofuse/276/console License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
…high-level interface + style: sort command list License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
It's better to have one mechanism for determining whether we're offline and to improve the SnR of this mechanism over time. We presently have too many arbitrary heuristics for determining whether we're running in offline mode. TRTTD is to use polymorphism to eliminate these conditional checks. (instantiate the node with offline versions of routing, network, etc.) It'll clean up the core constructor, make it easier to create ephemeral nodes, and eliminate a class of errors. @whyrusleeping @jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
the important change here is that within FindProvidersAsync, the channel is closed using a `defer`. This ensures the channel is always closed, regardless of the path taken to exit. + misc cleanup cc @whyrusleeping @jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
This commit makes GetProviders (sync) respect the request context. It also amends all of GetProviders' callsites to pass a context in. This meant changing the signature of the dht's handlerfunc. I think I'll start referring to the request context as Vito Corleone. cc @whyrusleeping @jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
When performing this "promise" pattern, it is important to provide a channel with space for one value. Otherwise the sender may block forever in the case of a receiver that decides to abandon the request. A subtle detail, but one that is important for avoiding leaked goroutines. cc @whyrusleeping @jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com> License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
…onously License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@jbenet License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@jbenet must be getConns to avoid clash with private var License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
btc
force-pushed
the
misc/2014-11-11th-hour
branch
from
e33e89a to
f870948
Compare
miscellaneous +/- (commands, dht, net)
a shared branch of innocuous miscellaneous fixes. Feel free to push.
Will be rebased after Dec 1, 01:00 UTC and merged.