Repository: markus-wa/demoinfocs-golang
Pull request #224 (2020-06-07 11:06:01)

core: add Parser.Close() to fix memory leak (#216) [backport to v1]
`Parser.Close()` will call `msgDispatcher.RemoveAllQueues()` to terminate the goroutine handling net-messages.
It needs to be called by the user before discarding the Parser.

codecov[bot]: # [Codecov](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=h1) Report
> Merging [#224](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=desc) into [v1](https://codecov.io/gh/markus-wa/demoinfocs-golang/commit/239ae3e8b1552e9ab3884bc25dc8928441e9ad31&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `90.00%`.

[![Impacted file tree graph](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/graphs/tree.svg?width=650&height=150&src=pr&token=20emCaZRHz)](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##               v1     #224      +/-   ##
==========================================
- Coverage   92.51%   92.50%   -0.02%     
==========================================
  Files          28       28              
  Lines        2659     2668       +9     
==========================================
+ Hits         2460     2468       +8     
- Misses        148      149       +1     
  Partials       51       51              
```


| [Impacted Files](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [parsing.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-cGFyc2luZy5nbw==) | `88.19% <50.00%> (-0.49%)` | :arrow_down: |
| [examples/entities/entities.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-ZXhhbXBsZXMvZW50aXRpZXMvZW50aXRpZXMuZ28=) | `92.59% <100.00%> (+0.28%)` | :arrow_up: |
| [examples/heatmap/heatmap.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-ZXhhbXBsZXMvaGVhdG1hcC9oZWF0bWFwLmdv) | `94.44% <100.00%> (+0.15%)` | :arrow_up: |
| [examples/nade-trajectories/nade\_trajectories.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-ZXhhbXBsZXMvbmFkZS10cmFqZWN0b3JpZXMvbmFkZV90cmFqZWN0b3JpZXMuZ28=) | `94.94% <100.00%> (+0.05%)` | :arrow_up: |
| [examples/net-messages/netmessages.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-ZXhhbXBsZXMvbmV0LW1lc3NhZ2VzL25ldG1lc3NhZ2VzLmdv) | `89.47% <100.00%> (+0.58%)` | :arrow_up: |
| [examples/print-events/print\_events.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-ZXhhbXBsZXMvcHJpbnQtZXZlbnRzL3ByaW50X2V2ZW50cy5nbw==) | `89.13% <100.00%> (+0.24%)` | :arrow_up: |
| [parser.go](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224/diff?src=pr&el=tree#diff-cGFyc2VyLmdv) | `95.29% <100.00%> (+0.11%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=footer). Last update [239ae3e...b15c918](https://codecov.io/gh/markus-wa/demoinfocs-golang/pull/224?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 60377e7..dfdfd3f 100644
--- a/README.md
+++ b/README.md
@@ -52,6 +52,7 @@ func main() {
 	defer f.Close()
 
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	// Register handler on kill events
 	p.RegisterEventHandler(func(e events.Kill) {
diff --git a/examples/entities/entities.go b/examples/entities/entities.go
index 191e850..5441d6c 100644
--- a/examples/entities/entities.go
+++ b/examples/entities/entities.go
@@ -18,6 +18,7 @@ func main() {
 	defer f.Close()
 
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	p.RegisterEventHandler(func(events.DataTablesParsed) {
 		p.ServerClasses().FindByName("CWeaponAWP").OnEntityCreated(func(ent *st.Entity) {
diff --git a/examples/heatmap/heatmap.go b/examples/heatmap/heatmap.go
index c960833..58d4e25 100644
--- a/examples/heatmap/heatmap.go
+++ b/examples/heatmap/heatmap.go
@@ -34,6 +34,7 @@ func main() {
 	defer f.Close()
 
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	// Parse header (contains map-name etc.)
 	header, err := p.ParseHeader()
diff --git a/examples/nade-trajectories/nade_trajectories.go b/examples/nade-trajectories/nade_trajectories.go
index 09604e2..1ff4d19 100644
--- a/examples/nade-trajectories/nade_trajectories.go
+++ b/examples/nade-trajectories/nade_trajectories.go
@@ -46,6 +46,7 @@ func main() {
 	defer f.Close()
 
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	header, err := p.ParseHeader()
 	checkError(err)
diff --git a/examples/net-messages/netmessages.go b/examples/net-messages/netmessages.go
index 62906ba..95e345e 100644
--- a/examples/net-messages/netmessages.go
+++ b/examples/net-messages/netmessages.go
@@ -26,6 +26,7 @@ func main() {
 	}
 
 	p := dem.NewParserWithConfig(f, cfg)
+	defer p.Close()
 
 	// Register handler for BSPDecal messages
 	p.RegisterNetMessageHandler(func(m *msg.CSVCMsg_BSPDecal) {
diff --git a/examples/print-events/print_events.go b/examples/print-events/print_events.go
index cf453f1..e18541c 100644
--- a/examples/print-events/print_events.go
+++ b/examples/print-events/print_events.go
@@ -13,10 +13,12 @@ import (
 // Run like this: go run print_events.go -demo /path/to/demo.dem
 func main() {
 	f, err := os.Open(ex.DemoPathFromArgs())
-	defer f.Close()
 	checkError(err)
 
+	defer f.Close()
+
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	// Parse header
 	header, err := p.ParseHeader()
diff --git a/fake/parser.go b/fake/parser.go
index b67de30..ca28d4e 100644
--- a/fake/parser.go
+++ b/fake/parser.go
@@ -227,3 +227,9 @@ func max(numbers map[int][]interface{}) (maxNumber int) {
 func (p *Parser) Cancel() {
 	p.Called()
 }
+
+// Close is a mock-implementation of Parser.Close().
+// NOP implementation.
+func (p *Parser) Close() {
+	p.Called()
+}
diff --git a/parser.go b/parser.go
index e3825ee..f86cc39 100644
--- a/parser.go
+++ b/parser.go
@@ -31,6 +31,7 @@ Example (without error handling):
 
 	f, _ := os.Open("/path/to/demo.dem")
 	p := dem.NewParser(f)
+	defer p.Close()
 	header := p.ParseHeader()
 	fmt.Println("Map:", header.MapName)
 	p.RegisterEventHandler(func(e events.BombExplode) {
@@ -203,6 +204,12 @@ func (p *Parser) UnregisterNetMessageHandler(identifier dp.HandlerIdentifier) {
 	p.msgDispatcher.UnregisterHandler(identifier)
 }
 
+// Close closes any open resources used by the Parser (go routines, file handles).
+// This must be called before discarding the Parser to avoid memory leaks.
+func (p *Parser) Close() {
+	p.msgDispatcher.RemoveAllQueues()
+}
+
 func (p *Parser) error() (err error) {
 	p.errLock.Lock()
 	err = p.err
diff --git a/parser_interface.go b/parser_interface.go
index 9042898..e83e357 100644
--- a/parser_interface.go
+++ b/parser_interface.go
@@ -24,6 +24,7 @@ import (
 //
 // 	f, _ := os.Open("/path/to/demo.dem")
 // 	p := dem.NewParser(f)
+// 	defer p.Close()
 // 	header := p.ParseHeader()
 // 	fmt.Println("Map:", header.MapName)
 // 	p.RegisterEventHandler(func(e events.BombExplode) {
@@ -92,6 +93,9 @@ type IParser interface {
 	//
 	// The identifier is returned at registration by RegisterNetMessageHandler().
 	UnregisterNetMessageHandler(identifier dp.HandlerIdentifier)
+	// Close closes any open resources used by the Parser (go routines, file handles).
+	// This must be called before discarding the Parser to avoid memory leaks.
+	Close()
 	// ParseHeader attempts to parse the header of the demo and returns it.
 	// If not done manually this will be called by Parser.ParseNextFrame() or Parser.ParseToEnd().
 	//
diff --git a/parsing.go b/parsing.go
index 5881098..2b85b1a 100644
--- a/parsing.go
+++ b/parsing.go
@@ -91,6 +91,7 @@ func (p *Parser) ParseToEnd() (err error) {
 	defer func() {
 		// Make sure all the messages of the demo are handled
 		p.msgDispatcher.SyncAllQueues()
+		p.msgDispatcher.RemoveAllQueues()
 
 		// Close msgQueue
 		if p.msgQueue != nil {
@@ -166,6 +167,7 @@ func (p *Parser) ParseNextFrame() (moreFrames bool, err error) {
 
 		// Close msgQueue (only if we are done)
 		if p.msgQueue != nil && !moreFrames {
+			p.msgDispatcher.RemoveAllQueues()
 			close(p.msgQueue)
 		}
 


Test patch:
diff --git a/examples_test.go b/examples_test.go
index cc11b66..988d711 100644
--- a/examples_test.go
+++ b/examples_test.go
@@ -18,9 +18,11 @@ func ExampleParser() {
 	if err != nil {
 		panic(err)
 	}
+
 	defer f.Close()
 
 	p := dem.NewParser(f)
+	defer p.Close()
 
 	// Register handler on kill events
 	p.RegisterEventHandler(func(e events.Kill) {
@@ -28,10 +30,12 @@ func ExampleParser() {
 		if e.IsHeadshot {
 			hs = " (HS)"
 		}
+
 		var wallBang string
 		if e.PenetratedObjects > 0 {
 			wallBang = " (WB)"
 		}
+
 		fmt.Printf("%s <%v%s%s> %s\n", e.Killer, e.Weapon, hs, wallBang, e.Victim)
 	})
 
@@ -46,5 +50,6 @@ func TestExamplesWithoutOutput(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping long running test")
 	}
+
 	ExampleParser()
 }
diff --git a/parser_test.go b/parser_test.go
index 6fa8f0e..b944d54 100644
--- a/parser_test.go
+++ b/parser_test.go
@@ -114,3 +114,24 @@ func TestParser_SetError_Multiple(t *testing.T) {
 
 	assert.Same(t, err, p.error())
 }
+
+func TestParser_Close(t *testing.T) {
+	p := new(Parser)
+	q := make(chan interface{}, 1)
+
+	p.msgDispatcher.AddQueues(q)
+
+	called := false
+
+	p.msgDispatcher.RegisterHandler(func(interface{}) {
+		called = true
+	})
+
+	p.Close()
+
+	q <- "this should not trigger the handler"
+
+	p.msgDispatcher.SyncAllQueues()
+
+	assert.False(t, called)
+}