[DONE] Fix concurrently closing and writing to the same chan

[rcmgleite]

Problem

The go client spawns two goroutines: handlePackets and pendingRequestsReaper

They both write to client.IncomingMsgChan.
The problem is: the client.Disconnect method was closing the IncomingMsgChan
and there is no way for the handlePackets and pendingRequestReaper goroutines to know
that this happened.
When one of these 2 goroutines tries to write to the closed channel the application panics.

Solution

I'm not completely sure that just not closing the IncomingMsgChan is the correct solution
but in some articles like this there is a 'rule' to only close channels from the writer goroutine.

[coveralls]

Pull Request Test Coverage Report for Build 347

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.02%) to 75.335%

---

Files with Coverage Reduction	New Missed Lines	%
agent/agent.go	2	81.25%

Totals
Change from base Build 338:	0.02%
Covered Lines:	3537
Relevant Lines:	4695

---

💛 - Coveralls

[henrod]

That way won't you leak channels?

[rcmgleite]

@henrod is there a simple way to verify it?
I tried to monitor memory usage using the client with this fix and everything seemed normal

I found some articles stating that the GC would clean the channels on behalf of the application

[cscatolini]

pitaya/client/client.go

Line 207 in 724c8df

case <-c.closeChan:

closeChan is actually being used to stop these goroutines:

	go c.handlePackets()
	go c.pendingRequestsReaper()

so by not closing it you will have a leak

[felipejfc]

can't you solve this with a lock in both of the goroutines + the Disconnect() method?

[rcmgleite]

as @cscatolini pointed out, the closeChan must be closed to avoid leaking goroutines.
As for the IncomingMsgChan, it seems that not closing it would no be a problem.

@felipejfc I could. But isn't it simpler this way?

[felipejfc]

I guess we could leak channels by not closing them but in this case it doesn't seems like a reason for not doing it because we're only using this client for tests... (depending on how we implement pitaya-bot this may be a point of memory leak tho)

[rcmgleite]

@felipejfc I think channels are cleaned by the GC, no?
And for now we are using this client to implement the pitaya-bot. This problem was actually found while developing the pitaya bot.

[felipejfc]

After doing some research I guess it is @rcmgleite, you can merge this