Skip to content
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.

Sign up for GitHub

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

Jump to bottom

store/tikv: call Next() after copIterator closed lead to goroutine leak #5624

Merged
merged 2 commits into from
Jan 15, 2018
Merged

store/tikv: call Next() after copIterator closed lead to goroutine leak #5624

merged 2 commits into from
Jan 15, 2018

Conversation

tiancaiamao
Copy link
Contributor

After Close(), worker groutine receive signal from copIterator.finished and
exit directly, without writing any thing to taskCh.
Next() receives from taskCh and may hang forever, cause the caller goroutine
leak.

@shenli @coocood

@shenli
Copy link
Member

shenli commented Jan 11, 2018

Could you add a test case to cover this?

@shenli
Copy link
Member

shenli commented Jan 11, 2018

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

I'll try it tomorrow, anyway, github is down right now.

@tiancaiamao
Copy link
Contributor Author

It seems not so easy to reproduce. @shenli
I test with this code but no leak found:

func (s *testSuite) TestEarlyClose(c *C) {
	tk := testkit.NewTestKit(c, s.store)
	tk.MustExec("use test")
	tk.MustExec("create table earlyclose (id int primary key)")

	// Insert 3000 rows.
	var values []string
	for i := 0; i < 3000; i++ {
		values = append(values, fmt.Sprintf("(%d)", i))
	}
	tk.MustExec("insert earlyclose values " + strings.Join(values, ","))

	// Get table ID for split.
	dom := sessionctx.GetDomain(tk.Se)
	is := dom.InfoSchema()
	tbl, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("earlyclose"))
	c.Assert(err, IsNil)
	tblID := tbl.Meta().ID

	// Split the table.
	s.cluster.SplitTable(s.mvccStore, tblID, 500)

	for i:=0; i<2000; i++ {
		rss, err := tk.Se.Execute("select * from earlyclose")
		c.Assert(err, IsNil)
		rs := rss[0]
		_, err = rs.Next()
		c.Assert(err, IsNil)
		rs.Close()
	}
}

@tiancaiamao
Copy link
Contributor Author

/run-all-tests -tidb-test=release-1.0 -private-test=release-1.0

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=release-1.0 tikv=release-1.0 tidb-private-test=release-1.0

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/441 tidb-private-test=pr/4 tikv=release-1.0

@ngaut
Copy link
Member

ngaut commented Jan 12, 2018

Have you tried gofail?

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=release-1.0 tikv=release-1.0 tidb-private-test=release-1.0

@tiancaiamao
store/tikv: call Next() after copIterator Closed lead to goroutine leak
17c3e3d 
After Close(), worker groutine receive from copIterator.finished and
exit directly, without writing any thing to taskCh.
Next() receives from taskCh and may hang forever, cause the caller goroutine
leak.
@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=release-1.0 tikv=release-1.0 tidb-private-test=release-1.0

winkyao
winkyao reviewed Jan 13, 2018
View reviewed changes
store/tikv/coprocessor.go
for {
if it.curr >= len(it.tasks) {
// Resp will be nil if iterator is finished.
return nil, nil
}
task := it.tasks[it.curr]
resp, ok = <-task.respChan
resp, ok, closed = recvFromRespCh(task.respChan, it.finished)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just close the task.respChan in copIterator.Close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because send to a closed channel would panic.

zz-jason
zz-jason reviewed Jan 14, 2018
View reviewed changes
store/tikv/coprocessor.go
for {
if it.curr >= len(it.tasks) {
// Resp will be nil if iterator is finished.
return nil, nil
}
task := it.tasks[it.curr]
resp, ok = <-task.respChan
resp, ok, closed = recvFromRespCh(task.respChan, it.finished)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/closed/exit/ or s/exit/closed/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think closed is better, you see the comment line in the next line:
// Close() is already called, so Next() is invalid.

@zz-jason

@tiancaiamao
Copy link
Contributor Author

Test added.
I make it select * from t by mistake, which should be select * from t order by id.
@coocood @shenli

@coocood
Copy link
Member

coocood commented Jan 15, 2018

LGTM

1 similar comment
@shenli
Copy link
Member

shenli commented Jan 15, 2018

LGTM

@shenli shenli added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 15, 2018
@shenli shenli merged commit 19ca35b into pingcap:release-1.0 Jan 15, 2018
@tiancaiamao tiancaiamao deleted the release-1.0 branch January 15, 2018 15:21
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Jan 16, 2018
@tiancaiamao
store/tikv: call Next() after copIterator closed lead to goroutine le…
81f0318 
…ak (pingcap#5624)

After Close(), worker groutine receive from copIterator.finished and
exit directly, without writing any thing to taskCh.
Next() receives from taskCh and may hang forever, cause the caller goroutine
leak.
zz-jason pushed a commit that referenced this pull request Jan 16, 2018
@tiancaiamao @zz-jason
store/tikv: call Next() after copIterator closed lead to goroutine le…
0e61c85 
…ak (#5624) (#5654)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@winkyao winkyao winkyao left review comments

@shenli shenli shenli approved these changes

Assignees
No one assigned
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tiancaiamao @shenli @ngaut @coocood @zz-jason @winkyao