Disconnect refactor

[ptsneves]

The first work on refactoring the client towards getting insights on how to improve it.

[MattBrittan]

Thanks - however I will need you to sign the ECA before I'm able to accept your PR's (see the readme for more details).

[MattBrittan]

This will set c.status to disconnected meaning that if status != connected { will always be true...

[ptsneves]

Yep, but unless I am missing something subtle, Isnt this already the case in the current code?

if status == connected {
 c.setConnected(disconnected)
....
}
else {
   // Irrelevant print
   c.setConnected(disconnected)
}

This is exactly the kind of complication i am trying to untangle. Apparent branches which are not :)

[MattBrittan]

unless I am missing something subtle

It's not really subtle. The current code is:

status := atomic.LoadUint32(&c.status)
if status == connected {
	DEBUG.Println(CLI, "disconnecting")
	c.setConnected(disconnected)
        // Send DISCONNECT etc...
} else {
	WARN.Println(CLI, "Disconnect() called but not connected (disconnected/reconnecting)")
	c.setConnected(disconnected)
}

So this checks the status and IF the client is connected it sends a DISCONNECT packet (calling c.setConnected(disconnected) immediately after the check regardless of whether the client is connected or not).

Your change is:

c.setConnected(disconnected)
status := atomic.LoadUint32(&c.status)
if status != connected {
    // Do stuff...
}

This calls c.setConnected(disconnected) BEFORE checking the status so what you are effectively doing is setting status to disconnected and then, after changing the value, checking if status != connected (this will always be true because you just set it to disconnected). This can be summarised as:

c.status = disconnected
status := c.status
if status != connected {
    // As you have set status to disconnected this branch will ALWAYS be taken and the client will never disconnect cleanly
   return
}
// Unreachable code....

You could fix this by changing the order of your statements:

status := atomic.LoadUint32(&c.status)
c.setConnected(disconnected)
if status != connected {
}

However I'm not altogether convinced that this is easier to read (it does make the race condition more obvious but, as I mentioned in the issue, the way c.status is handled is far from ideal (the result of many changes over the years by a large number of contributors).

[MattBrittan]

Changes are needed before this can be accepted.

[MattBrittan]

Thanks for your contribution.