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
distsql: fix goroutine leak caused huge memory footprint #1834
Conversation
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.
Can you add an unit test for checking go routine leak?
@@ -131,6 +135,9 @@ func (r *selectResult) IgnoreData() { | |||
|
|||
// Close closes SelectResult. | |||
func (r *selectResult) Close() error { | |||
close(r.cancel) | |||
for _ = range r.results { |
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.
Add comment for this logic.
@@ -253,6 +269,8 @@ func (pr *partialResult) getChunk() *tipb.Chunk { | |||
|
|||
// Close closes the sub result. | |||
func (pr *partialResult) Close() error { | |||
for _ = range pr.done { |
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.
Add comment for this.
) | ||
|
||
// choose this style because error handle is really boring. | ||
for step := 0; err == nil && step < 3; step++ { |
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.
This style is not as clear as the old way.
@@ -99,7 +103,7 @@ func (r *selectResult) fetch() { | |||
ignoreData: r.ignoreData, | |||
done: make(chan error), |
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.
Make the done
channel with buffer length 1 may resolve the issue as well.
And this has minimum code change.
} | ||
} | ||
|
||
time.Sleep(3 * time.Second) |
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.
3 seconds seems too long for a single test case.
We can wait a few mili-seconds in a for loop, once the count has reduced to 5, we can break directly.
|
||
func (resp *mockResponse) Next() (io.ReadCloser, error) { | ||
resp.count++ | ||
if resp.count == 500 { |
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.
We don't need as much as 500 responses for this test case.
@@ -73,11 +73,14 @@ func (s *testTableCodecSuite) TestGoroutineLeak(c *C) { | |||
} | |||
} | |||
|
|||
time.Sleep(3 * time.Second) | |||
countAfter := runtime.NumGoroutine() | |||
for i := time.Duration(0); i < 3*time.Second; i = i + 10*time.Millisecond { |
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.
Forgot to Sleep?
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.
yes
LGTM |
@@ -43,3 +50,65 @@ func (s *testTableCodecSuite) TestColumnToProto(c *C) { | |||
pc := columnToProto(col) | |||
c.Assert(pc.GetFlag(), Equals, int32(10)) | |||
} | |||
|
|||
// For Issue 1791 |
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.
s/Issue/issue ?
LGTM |
fix a bug in #1791
there may still other unknown problems, I'll test tomorrow.