Skip to content

Conversation

tanordheim
Copy link
Contributor

Proposed changes

As discussed in #358, there is an issue that can sometimes manifest where a charge point reconnects and the connection tracking still believes the old connection is active - causing it to drop the new incoming connection. This is an issue that over time will end up with no chargers actually connected, and no data streaming in from the chargers. See this comment for a more elaborate description of the issue.

This change allows setting the duplicate connection behavior on the ws.Server by calling:

server.SetDuplicateConnectionBehavior(ws.DuplicateConnectionBehaviorKeepNew) // or the default value ws.DuplicateConnectionBehaviorKeepCurrent

When this value is set to DuplicateConnectionBehaviorKeepNew the existing connection will be closed and the new connection will be allowed, solving the issue described in #358.

As mentioned in this comment by @xBlaz3kx, this can potentially have security implications - especially when running without auth - where you would be able to essentially force-disconnect and hijack charger connections by creating new malicious connections towards existing connected charge points. For some use cases this is OK, and with this change its at least up to the consuming service to decide what works best. I've tried highlighting these security concerns in the code.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of
them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before
merging your code.

  • [x I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

@tanordheim tanordheim force-pushed the prefer-new-connection-over-old-on-duplicate branch from c2c110d to 6944194 Compare August 15, 2025 09:19
@tanordheim tanordheim force-pushed the prefer-new-connection-over-old-on-duplicate branch from 6944194 to 76caa5b Compare August 15, 2025 09:23
_ = currentConn.connection.WriteControl(websocket.CloseMessage,
websocket.FormatCloseMessage(websocket.ClosePolicyViolation, "a connection with this ID has reconnected"),
time.Now().Add(s.timeoutConfig.WriteWait))
_ = currentConn.connection.Close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick question; am I understanding the codeflow correctly here when I assume that the entry from s.connections for the chargepoint ID will be removed from the map as a result of me simply closing the connection here, so the new entry will be added correctly after passing this switch statement?

Or is that onClosed callback on the somehow not synchronous, risking that the entry from s.connections for that charge point ID is removed from the map after the new connection has been accepted?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm afraid it's the latter:

  1. currentCon.connection.Close() tells the underlying websocket to close
  2. the cleanup closure is invoked asynchronously
  3. the callback eventually deletes the element from the map in server.go/handleDisconnect

So the new element will (in most cases) be deleted right after being added. To verify add these lines to the end of the unit test:

s.Equal(1, len(s.server.connections))
testWS, ok := s.server.connections["testws"]
s.True(ok, "expected testws connection to exist")
s.NotNil(testWS, "expected testws connection to be non-nil")

I guess we would need a dedicated method to force-close a websocket without triggering any callback

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.

2 participants