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.

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

Goroutine leak with latest "cancelable stdin" changes #76

Closed
knz opened this issue Sep 4, 2016 · 3 comments · Fixed by #77 or #78
Closed

Goroutine leak with latest "cancelable stdin" changes #76

knz opened this issue Sep 4, 2016 · 3 comments · Fixed by #77 or #78
Assignees

Comments

@knz
Copy link

knz commented Sep 4, 2016

See: https://circleci.com/gh/cockroachdb/cockroach/22259

This prevents us at CockroachDB from upgrading to the latest readline.

The problem is that the global var Stdin is set to a CancelableStdin object but this object is never closed, so there is a goroutine remaining when the program using readline terminates. This makes leak checkers unhappy.

The better solution would be to create an API for readline to "initialize" and "finalize" the entire library. The CancelableStdin object would be created in "initialize" and closed in "finalize". This way, if a program (like cockroach) merely imports the readline package but does not actually use it, there will be no goroutine created.

@chzyer chzyer mentioned this issue Sep 4, 2016
@chzyer chzyer self-assigned this Sep 4, 2016
@chzyer chzyer closed this as completed in #77 Sep 4, 2016
@chzyer chzyer removed the in progress label Sep 4, 2016
@chzyer
Copy link
Owner

chzyer commented Sep 4, 2016

Also, add defer ins.Close() here should fix this.
https://github.com/cockroachdb/cockroach/blob/master/cli/sql.go#L247

@knz
Copy link
Author

knz commented Sep 4, 2016

Thanks for the hint. However the failure was unrelated to the sql client, it occurs also when we run a Go test for the cli package. The leak occured because cli links to readline and the ioloop goroutine was started even if the test didn't use readline.

@chzyer
Copy link
Owner

chzyer commented Sep 4, 2016

I knew that, so I fix that in #77, but I just have a better fix in #78.
You can check the diff in here.
And, It also needs you add defer ins.Close() in sql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants