Improve Session Management for Charge Point Connections #2908

Sn0w3y · 2024-12-03T23:07:32Z

Summary

This PR addresses the issue of improper session management when multiple connections are established to the same charge point. Specifically, it ensures that only one active session per charge point is maintained, and existing sessions are properly terminated before establishing new ones. This update helps prevent unintended side effects where disconnecting an older connection impacts newer connections.

Changes Made

Handle Existing Sessions When a New Connection is Established
- Updated newSession method in MyJsonServer.java:
  - Checks if an existing session already exists for a given charge point.
  - If found, gracefully terminates the old session by invoking lostSession() and removes it from active sessions.
  - Associates the new session with the charge point and initializes it accordingly.
Prevent lostSession from Affecting New Connections
- Updated lostSession method in MyJsonServer.java:
  - Ensures that only the intended session is terminated without affecting other active connections.
  - Before removing a session from ocppSessions, it verifies that the session being disconnected is still the active one.
Ensure Proper Cleanup in removeEvcs
- Reviewed and modified removeEvcs method in EvcsOcppServer.java:
  - Ensures that removing an EVCS component correctly handles session mappings without affecting other active sessions.
  - Properly removes the EVCS component from session lists and invokes lostSession() for cleanup.

Additional Enhancements

Thread Safety: Made sure that shared resources such as activeEvcsSessions and ocppSessions are accessed in a thread-safe manner.
Logging Improvements: Enhanced logging to include more contextual information to facilitate easier debugging and monitoring of session activities.
Session Validation: Added additional checks to validate the integrity of sessions to ensure correct mapping between charge points and sessions.
Resource Cleanup: Ensured proper resource cleanup upon session termination to prevent memory leaks and stale references.

Testing

Please test this Code Improvement as i do not have the ability to test it on my side.
@katsuya please take this task maybe as you have the issue right now in:
OCPP server fail to send commands to the charging stations when two connections are established #2906

Motivation

These changes address the observed problem where OpenEMS failed to handle multiple simultaneous connections to the same charge point, causing instability in session management. By implementing these changes, the system will now correctly manage and isolate sessions, ensuring reliable operation of charge point connections.

Related Issues

Fixes improper handling of multiple charge point connections.
Resolves issues with old connections impacting newly established sessions.

Notes

If further refinements are needed, or any issues persist with session handling, please feel free to provide feedback.

codecov · 2024-12-03T23:10:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #2908      +/-   ##
=============================================
+ Coverage      56.63%   56.64%   +0.01%     
- Complexity      9513     9515       +2     
=============================================
  Files           2253     2253              
  Lines          96074    96074              
  Branches        7095     7095              
=============================================
+ Hits           54401    54408       +7     
+ Misses         39666    39658       -8     
- Partials        2007     2008       +1

katsuya · 2024-12-04T00:14:11Z

@Sn0w3y Thanks a lot! We'll give the provided patch a try.

Thread Safety: Made sure that shared resources such as activeEvcsSessions and ocppSessions are accessed in a thread-safe manner.

This is absolutely true. Occasionally, we also experience erratic behavior, and it's sometimes difficult to determine if this is the cause, which can be quite frustrating.

By the way, here’s the fix we implemented. It mostly resolved the issue, but we’re still experiencing some very rare instances of erratic behavior, which is troubling.

From 342832e40892ae147dc020edd37d93fc297b52e1 Mon Sep 17 00:00:00 2001
From: Katsuya Oda <[email protected]>
Date: Wed, 4 Dec 2024 09:05:59 +0900
Subject: [PATCH] fix: remove previous session

---
 .../src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java  | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java b/io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java
index 66d4d78b3..e461755a2 100644
--- a/io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java
+++ b/io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java
@@ -90,7 +90,7 @@ public class MyJsonServer {
 
 				var ocppIdentifier = information.getIdentifier().replace("/", "");
 
-				MyJsonServer.this.parent.ocppSessions.put(ocppIdentifier, sessionIndex);
+				var sessionIndexPrevious = MyJsonServer.this.parent.ocppSessions.put(ocppIdentifier, sessionIndex);
 
 				var presentEvcss = MyJsonServer.this.parent.ocppEvcss.get(ocppIdentifier);
 
@@ -103,6 +103,10 @@ public class MyJsonServer {
 					evcs.newSession(MyJsonServer.this.parent, sessionIndex);
 					MyJsonServer.this.sendInitialRequests(sessionIndex, evcs);
 				}
+
+				if (sessionIndexPrevious != null) {
+					MyJsonServer.this.parent.activeEvcsSessions.remove(sessionIndexPrevious);
+				}
 			}
 
 			@Override
-- 
2.39.5 (Apple Git-154)

Sn0w3y · 2024-12-04T00:25:44Z

By the way, here’s the fix we implemented. It mostly resolved the issue, but we’re still experiencing some very rare instances of erratic behavior, which is troubling.

@katsuya Thank you for your efforts in addressing the session management issue aswell!
However, my fix provides a more comprehensive solution by not only removing the previous session but also invoking lostSession() to ensure complete cleanup.
Additionally, it includes verification to confirm that only the intended session is terminated, enhancing thread safety and preventing any unintended impact on new connections.

Greetings

katsuya · 2024-12-10T00:21:02Z

io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/EvcsOcppServer.java

+				evcss.remove(ocppEvcs);
+				if (evcss.isEmpty()) {
+					this.activeEvcsSessions.remove(sessionId);
+					// Also remove from ocppSessions if needed


In actual production environments, we rarely remove components, so we went ahead with weekend testing without including this commit. However, I don’t see any issues with the code itself.

Could you maybe still test it ?

katsuya · 2024-12-10T00:27:14Z

io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java

@@ -90,6 +90,23 @@ public void newSession(UUID sessionIndex, SessionInformation information) {

 				var ocppIdentifier = information.getIdentifier().replace("/", "");

+				// Check if there is already a session for this ocppIdentifier


Over the weekend, we tested both our patch(#2908 (comment)) and this one, and both appeared to work properly. However, I’m concerned about the following point in this patch. What do you think?

The old session is deleted before establishing a new one(https://github.com/OpenEMS/openems/pull/2908/files#diff-a9d06149a614c84998d6bcad6d51f36ca40834f2e200120f2a2e9bfb290601eaR119). If something goes wrong during that process, wouldn’t we end up unable to use both the old and new sessions?

katsuya · 2024-12-10T00:32:30Z

io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java

@@ -122,10 +139,16 @@ public void lostSession(UUID sessionIndex) {
 				for (Entry<String, UUID> session : MyJsonServer.this.parent.ocppSessions.entrySet()) {


To address the issue mentioned below, we added the following changes. However, these modifications might not be necessary if our only concern is the brief window between establishing the new session and discarding the old one, as they make the code more complex.

When the old connection is disconnected, it should ensure that the lostSession for the charge point referenced by the new connection is not invoked.

if (sessionEvcss != null) { for (AbstractManagedOcppEvcsComponent ocppEvcs : sessionEvcss) { var ocppId = ocppEvcs.getConfiguredOcppId(); UUID latest = MyJsonServer.this.parent.ocppSessions.get(ocppId); /** * Since multiple sessions are connected with the same ocppId, if * `lostSession` is executed with an old session, the `sessionId` of the * component corresponding to the latest session would be set to null. */ if (latest != sessionIndex) { MyJsonServer.this.logWarn("skip lostSession of old session [" + sessionIndex + "] for ocppIdentifier [" + ocppId + "]"); continue; } ocppEvcs.lostSession(); MyJsonServer.this.logWarn("lostSession [" + sessionIndex + "] for ocppIdentifier [" + ocppId + "]"); } MyJsonServer.this.parent.activeEvcsSessions.remove(sessionIndex); MyJsonServer.this.logWarn("remove activeEvcsSessions [" + sessionIndex + "]"); }

katsuya · 2024-12-10T00:35:10Z

io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java

 					}
 				}

-				MyJsonServer.this.parent.ocppSessions.remove(ocppId);
+				// Before removing, check if the sessionIndex is still the one mapped to ocppId


This code also deals with the window time between establishing the new session and discarding the old one inside newSession, so it might not be necessary after all. During our weekend tests, we couldn’t hit this code path without artificially introducing sleeps, which suggests we may not need it.

katsuya

@Sn0w3y It seems that the fix for the following issue has been removed and replaced with the changes from #2912. Was this intentional? Since the root causes are different, I think it would be better to address them separately.

When a new connection is established, the old connection must be correctly removed from activeEvcSessions.

Sn0w3y · 2024-12-19T01:48:56Z

@katsuya you are invited to do the WIP please :)

Sn0w3y added 2 commits December 4, 2024 00:02

Update EvcsOcppServer.java

f94ac47

Update MyJsonServer.java

7213b92

Sn0w3y added 2 commits December 4, 2024 00:31

Checkstyle Fix

abb988c

Fix super class missing

25e7484

katsuya mentioned this pull request Dec 10, 2024

OCPP server fail to send commands to the charging stations when two connections are established #2906

Open

katsuya reviewed Dec 10, 2024

View reviewed changes

Sn0w3y requested a review from katsuya December 16, 2024 16:03

katsuya reviewed Dec 17, 2024

View reviewed changes

Sn0w3y force-pushed the fix-ocpp-server branch from 5d3f2d5 to 25e7484 Compare December 19, 2024 01:47

Merge branch 'OpenEMS:develop' into fix-ocpp-server

c0a9afa

Sn0w3y marked this pull request as draft December 20, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Session Management for Charge Point Connections #2908

Improve Session Management for Charge Point Connections #2908

Sn0w3y commented Dec 3, 2024

codecov bot commented Dec 3, 2024 •

edited

Loading

katsuya commented Dec 4, 2024 •

edited

Loading

Sn0w3y commented Dec 4, 2024 •

edited

Loading

katsuya Dec 10, 2024

Sn0w3y Dec 16, 2024

katsuya Dec 10, 2024

katsuya Dec 10, 2024

katsuya Dec 10, 2024

katsuya left a comment

Sn0w3y commented Dec 19, 2024

		@@ -90,6 +90,23 @@ public void newSession(UUID sessionIndex, SessionInformation information) {

		var ocppIdentifier = information.getIdentifier().replace("/", "");

		// Check if there is already a session for this ocppIdentifier

		@@ -122,10 +139,16 @@ public void lostSession(UUID sessionIndex) {
		for (Entry<String, UUID> session : MyJsonServer.this.parent.ocppSessions.entrySet()) {

Improve Session Management for Charge Point Connections #2908

Are you sure you want to change the base?

Improve Session Management for Charge Point Connections #2908

Conversation

Sn0w3y commented Dec 3, 2024

Summary

Changes Made

Additional Enhancements

Testing

Motivation

Related Issues

Notes

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

katsuya commented Dec 4, 2024 • edited Loading

Sn0w3y commented Dec 4, 2024 • edited Loading

katsuya Dec 10, 2024

Choose a reason for hiding this comment

Sn0w3y Dec 16, 2024

Choose a reason for hiding this comment

katsuya Dec 10, 2024

Choose a reason for hiding this comment

katsuya Dec 10, 2024

Choose a reason for hiding this comment

katsuya Dec 10, 2024

Choose a reason for hiding this comment

katsuya left a comment

Choose a reason for hiding this comment

Sn0w3y commented Dec 19, 2024

codecov bot commented Dec 3, 2024 •

edited

Loading

katsuya commented Dec 4, 2024 •

edited

Loading

Sn0w3y commented Dec 4, 2024 •

edited

Loading