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

[tiancaiamao]

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]

Could you add a test case to cover this?

[shenli]

/run-all-tests

[tiancaiamao]

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

[tiancaiamao]

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]

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

[tiancaiamao]

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

[tiancaiamao]

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

[ngaut]

Have you tried gofail?

[tiancaiamao]

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

[tiancaiamao]

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

[winkyao]

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

[tiancaiamao]

Because send to a closed channel would panic.

[zz-jason]

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

[tiancaiamao]

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]

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

[coocood]

LGTM

[shenli]

LGTM