Skip to content
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

Disconnect refactor #586

Merged
merged 3 commits into from Mar 13, 2022
Merged

Conversation

ptsneves
Copy link
Contributor

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

This makes it easy to refactor the conditional and makes sure
that it always gets executed.
@MattBrittan
Copy link
Contributor

MattBrittan commented Feb 11, 2022

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).

client.go Outdated
WARN.Println("Disconnect packet not sent due to timeout")
}
defer c.disconnect()
c.setConnected(disconnected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ptsneves ptsneves Feb 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@MattBrittan MattBrittan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are needed before this can be accepted.

c.setConnected(disconnected) was called inconditionally in both
conditions start so it is in fact independent of the status. Move
it out.
the diconnectSent logic was being used in it's own conditional when
it naturally is not conditional when the writing to c.ouboundP is
successful.

Signed-off-by: Paulo Neves <ptsneves@gmail.com>
@MattBrittan MattBrittan merged commit eaac59b into eclipse:master Mar 13, 2022
@MattBrittan
Copy link
Contributor

Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants