-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
@whyrusleeping Fixed the fix to actually fix what needed to be fixed. +Added a test for it |
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.
Oh wow, great catch here. Thanks @magik6k :)
limiter_test.go
Outdated
if l.fdConsuming < 0 { | ||
t.Fatalf("l.fdConsuming < 0") | ||
} | ||
fmt.Println(dials) |
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.
Best to remove the log here or use t.Log
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.
Didn't notice when doing commit, removed
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.
LGTM (test failure looks related to GitHub's outage).
This fixes ipfs/kubo#4102
With this fix when running daemon SYN_SENT connections are hovering around 150-200, which is a huge improvement from around 3000.
Unit tests are TODO