Explicitly stop the timer when the connection is closed.

[notsure2]

Fixes #137 again.

[cbeuw]

This won't work either, because the goroutine started on each inocation of broadcastAfter() will not actually return on timer.Stop() (it doesn't actually close the channel). I'm trying to see if there is a solution that involves a single long living broadcaster goroutine

[notsure2]

Then how do you cancel an in-flight timer ? I am under the impression that calling timer.Stop and then calling broadcast will wake the goroutine, which will call Stop again, which will return false, and then it will then drain the channel and terminate...

Or am I mistaken ? I'm not very familiar, so any correction will be appreciated.

EDIT: Yeah, I tested, it doesn't work

[codecov]

Codecov Report

Merging #138 (0327d0f) into master (c0040f2) will increase coverage by 0.09%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   67.68%   67.77%   +0.09%     
==========================================
  Files          37       37              
  Lines        2392     2402      +10     
==========================================
+ Hits         1619     1628       +9     
- Misses        610      611       +1     
  Partials      163      163              

Impacted Files	Coverage Δ
internal/multiplex/datagramBufferedPipe.go	84.88% <82.35%> (-0.31%)	⬇️
internal/multiplex/streamBufferedPipe.go	93.24% <88.23%> (+0.48%)	⬆️

---

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 c0040f2...0327d0f. Read the comment docs.

[cbeuw]

Then how do you cancel an in-flight timer

I don't think golang's time module provides any convenient way of doing this, because timer.C only ever gets filled when the timer fires. There's no way of unblocking a listener other than making the timer fire. One way is to have another channel that gets filled inside Close(), and put this channel and the timer channel together in a select statement:

for {
	select {
               	case <-p.timer.C:
               	p.rwCond.Broadcast()
		case <-p.closeCh:
		return
	}
}

Alternatively, we could probably do this

for {
	<-p.timer.C
	if p.closed { // Checking this is technically a race condition, but this can be easily solved by using atomics
		return
	} else {
		p.rwCond.Broadcast()
	}
}

And in Close() we do p.timer.Reset(0) so the above listener will get unblocked immediately. It seems to work but I need to do more tests

[notsure2]

Even if you unblock the goroutine by selecting on another channel that gets filled during close, don't you still need to clean up the timer (there's a runtimeTimer inside) and a channel also inside that need be cleaned up, otherwise it's still leaking...

I think I prefer the p.timer.Reset(0) solution much more. Cloak needs to be able to run in routers with tiny ram and allocating as less things as possible is preferred.

https://stackoverflow.com/questions/50223771/how-to-stop-a-timer-correctly

[notsure2]

OK i implemented the second channel solution (in my updated push). But memory usage is still climbing... Just that the profile now changed. I will update the bug report. #137

[notsure2]

@cbeuw Please check the updated branch, the issue is much much better now, almost solved. I'll leave it running for long time to confirm it.

[notsure2]

Memory use is even better now, only 25 MB RAM usage stable with 110 active sessions!

[cbeuw]

That's great to hear. Thanks a lot for the help!