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

txt parser: fix goroutine leak #570

Merged
merged 2 commits into from Nov 17, 2017

Conversation

Projects
None yet
2 participants
@miekg
Owner

miekg commented Nov 16, 2017

When a higher level (grammar or syntax) error was encountered the lower
level zlexer routine would be left open. This leaks a goroutine.

This PR fixes this by signalling this error, retrieving any remaining
items from the channel, so zlexer can return.

Fixes #586
Fixes coredns/coredns#1233

@codecov-io

This comment has been minimized.

codecov-io commented Nov 16, 2017

Codecov Report

Merging #570 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #570      +/-   ##
=========================================
+ Coverage    58.3%   58.4%   +0.09%     
=========================================
  Files          37      37              
  Lines        9865    9883      +18     
=========================================
+ Hits         5752    5772      +20     
+ Misses       3064    3063       -1     
+ Partials     1049    1048       -1
Impacted Files Coverage Δ
scanner.go 100% <100%> (ø) ⬆️
scan.go 80.48% <100%> (+0.17%) ⬆️
dnssec_keyscan.go 71.26% <100%> (+0.85%) ⬆️
scan_rr.go 70.06% <0%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fc4eb2...30c3c85. Read the comment docs.

@miekg

This comment has been minimized.

Owner

miekg commented Nov 17, 2017

The

_, ok := <-c
 +		_, ok = <-c
 +		if !ok {
 +			// too bad
 +		}

is ugly, but I don't have a better idea. I'm pondering on adding a test (or figuring out one)

@miekg

This comment has been minimized.

Owner

miekg commented Nov 17, 2017

Added leaktest:

this branch:

=== RUN   TestParseBadNAPTR
--- PASS: TestParseBadNAPTR (0.01s)
PASS
ok  	github.com/miekg/dns	1.022s

master branch with test copied in:

=== RUN   TestParseBadNAPTR
1 instances of:
github.com/miekg/dns.zlexer(0xc42005c080, 0xc42001c060)
	/home/miek/g/src/github.com/miekg/dns/scan.go:602 +0x52a
created by github.com/miekg/dns.parseZone
	/home/miek/g/src/github.com/miekg/dns/scan.go:185 +0x297
--- FAIL: TestParseBadNAPTR (0.52s)
	parse_test.go:1417: leaked goroutines: too many goroutines running after dns test(s)
FAIL
exit status 1
FAIL	github.com/miekg/dns	0.534s
txt parser: fix goroutine leak
When a higher level (grammar or syntax) error was encountered the lower
level zlexer routine would be left open and trying to send more tokens
on the channel c. This leaks a goroutine, per failed parse...

This PR fixes this by signalling this error - by canceling a context -
retrieving any remaining items from the channel, so zlexer can return.

It also adds a goroutine leak test that can be re-used in other tests,
the TestParseBadNAPTR test uses this leak detector.

The private key parsing code had the same bug and is also fixed in this
PR.

Fixes #586
Fixes coredns/coredns#1233

@miekg miekg force-pushed the leak branch from 48f9eff to ac98d1d Nov 17, 2017

@miekg miekg merged commit cfe4128 into master Nov 17, 2017

4 checks passed

codecov/patch 100% of diff hit (target 58.3%)
Details
codecov/project 58.4% (+0.09%) compared to 9fc4eb2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@miekg miekg deleted the leak branch Nov 17, 2017

@tmthrgd tmthrgd referenced this pull request Oct 14, 2018

Merged

Eliminate lexer goroutines #792

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment