Cleanup, Metrics, Closed Leaks #1
Conversation
| wmgr := &wantManager{ | ||
| bs: bs, | ||
| wants: make(map[string]int64), | ||
| info: log, | ||
| } | ||
|
|
||
| for i, o := range opts { |
There was a problem hiding this comment.
I think we should use proper typed functional options for this. I started working on this, so if you want I can push it.
There was a problem hiding this comment.
Agreed in general but I wasn't sure if I want to keep them anyways... I'm still not sure about the best way to partially inject these counters. It's really easy to wrap a whole interface but in some cases that isn't possible.
My biggest dislike is all the if gauge != nil { .Add(1) } lines which I want to place with functions at least before merging this. I'm all yours for better ideas.. :)
There was a problem hiding this comment.
Hmm, I think we'll have to discuss the partial injection in person, I don't really know what you mean by it. But I see what you mean about the conditional adds... I don't know the prometheus API enough to comment on that, but in general it would be nice not to have a hard dependency on prometheus... maybe adding hooks or something like that is a better approach?
|
|
||
| // TODO: ctx?? this pours into the broadcast, right? |
There was a problem hiding this comment.
You're right, this function should probably take a context...
| @@ -26,8 +28,14 @@ import ( | |||
| "go.cryptoscope.co/ssb/repo" | |||
| ) | |||
|
|
|||
| func ckFatal(err error) { | |||
There was a problem hiding this comment.
Why are you exiting instead of using assert/require/testing.T?
There was a problem hiding this comment.
you can't use t.Error / t.Fatal inside of goroutines. So neither assert or require. There might be a nicer way using chan err or something but I wanted understandable behavior first.
See golang/go#17900 or ipfs/kubo#2043
There was a problem hiding this comment.
Ahh, I see. Yeah I think we should use an chan error for that, but I see why you did it how you did. Not sure what the best way is - maybe let everything run in goroutines in the test goroutine just read the error chans that all the goroutines write to?
There was a problem hiding this comment.
yea sounds good but let's make this a meta-todo for all the repos then.
There was a problem hiding this comment.
I'll gently start the effort by opening an issue to open issues everywhere...
plugins/gossip/pour.go
Outdated
| @@ -19,17 +19,20 @@ func (h *handler) pourFeed(ctx context.Context, req *muxrpc.Request) error { | |||
| return errors.New("not enough arguments, expecting feed id") | |||
| } | |||
| qv := req.Args[0].(map[string]interface{}) | |||
| if id, ok := qv["id"]; ok && id == `@S9lG7keeOfIAepU1LRD9XUDtuesbejRC2+3/FRaGZ3s=.sha256` { | |||
There was a problem hiding this comment.
Can we not hardcode this, and instead just drop errors like this in general? It's super easy to add more like this to the system, and I don't want to maintain an official list of broken messages.
There was a problem hiding this comment.
Sorry but I don't see the alternative, if you are suggesting one? I don't want to spam our logs with this.
There was a problem hiding this comment.
Hm, maybe just swallow the error from ParseFeedRef and silently abort if it fails? Or better, return an error if it does not have the structure of a feed ref, but silently swallow errors on unknown/ridiculous algorithms. Do that by using typed errors in ParseFeedRef.
There was a problem hiding this comment.
phew... don't see much point in that complexity either. I changed it to ignore everything that isn't a valid feed ref now.
There was a problem hiding this comment.
SGTM. My thinking was that a generic "process a message" function maybe shouldn't silently abort, but this error also should not be printed every time it happens. That was what I came up with to make this distinguishable.
i dont remember how to use the returned wait func...
this is why no messages got out - jsbot calles with live:true limit:0 margeret just said "great! here: have no messages!"
TODO: there might be a missing cancel in the context setup
most of the times this is just n=0
also makes inconsistant log a shutdown error
unified connWrapper handling if we return out of the loop we stop accepting new connections after the first error (timeout or kex fail).
add seperate connect command to sbotcli also adds whoami and blobs commands to the control handler
|
merged manually |
All the good stuff. Making this a PR so I can properly review.