Fix the GOPACS handler and redispatch poller lifecycle - #100
Merged
Merged
Conversation
GOPACSHandler kept its ResteasyClient in a field but undeploy() only cancelled the scheduled futures and undeployed the web service. Every update of an EmsGOPACSAsset does stop-then-start, so each edit left the previous client and its connection pool allocated. Closing is safe with the shared executor: createClient(ExecutorService) goes through ResteasyClientBuilder.executorService, which leaves Container.EXECUTOR untouched. The constructor now closes the client if client.target(...) throws between createClient and the end of construction, since no reference escapes a constructor that threw and undeploy() could never run. Scheduling now goes through a single schedule() helper that records every ScheduledFuture. Previously only the futures from processRawMessage were tracked; the power updates scheduled by schedulePowerUpdate were not, so they survived undeploy(). The list is synchronized because tasks are scheduled from Undertow request threads and cancelled from the service thread, and completed futures are pruned on each call so the list does not grow for the handler's lifetime. Tests cover: undeploy on a test-support handler is a no-op, undeploy cancels every pending future exactly once, and completed futures are pruned on the next schedule call.
GOPACSHandler deployed its JAX-RS application as the last step of its constructor. The deployed resource routes straight to processRawMessage, so the handler was reachable from Undertow request threads before the constructor had returned, and the Java memory model gives no guarantee about what those threads see of an object whose reference escaped mid-construction. deploy() is now a public no-arg method called after construction, the same shape DistroEnergyHandler and GOPACSRedispatchHandler.startPolling already use. EmsOptimisationService registers the handler in its map before calling deploy(), so a deployment that throws still leaves the handler reachable from stop() and its client gets closed.
EmsOptimisationService.stop() cancelled the redispatch pollers and the optimisation timers but never touched gopacsHandlerMap, so the GOPACS handlers kept their JAX-RS deployment and REST client across a service stop. Undeploy and clear them alongside the other handler maps.
Covers processAssetChange for EmsGOPACSAsset CREATE, UPDATE and DELETE and the new stop() teardown, through a recording GOPACSHandler subclass built by the real constructor (fake Container with OAuth config and a temporary private-key file) whose deploy()/undeploy() only count calls. Same pattern as EmsOptimisationServiceDistroEnergyTest; no mocking of the concrete class and no extra test dependency.
processAssetChange stopped the handler on UPDATE with the EAN from the updated entity, but gopacsHandlerMap is keyed by EAN and the entity carries the new value. When the contracted EAN was edited through the asset editor the stop was a silent no-op, so the handler registered under the old EAN kept its JAX-RS deployment and REST client alive next to the new one. The attribute-event path already handles this case with the old value, but an asset merge does not emit attribute events. Undeploy every handler deployed for the asset id instead, whichever EAN it is registered under. GOPACSHandler exposes the asset id it was deployed for; the field name (electricitySupplierAssetId) predates the EmsGOPACSAsset type and is left alone. The lifecycle test gains the changed-EAN case, which failed before this change.
processAssetChange left the redispatch poller untouched on UPDATE, on the grounds that it is managed through attribute events. An asset merge emits none, so editing the contracted EAN through the asset editor left the poller running under the old EAN: it kept polling and resolving EAN effectivity for a congestion point the asset no longer has, and there was no key it could be stopped by afterwards. Unlike the GOPACS handler, the poller is not simply restarted on every save. Its announcement bookkeeping (lastProcessedAnnouncementId and the set of recorded announcement ids) is in memory only and history entries are not deduplicated against the asset, so a restart re-records every open announcement. UPDATE therefore only stops a poller registered for the asset under an EAN other than the current one, and starts a new one under the current EAN if redispatch is still enabled. A poller already running under the current EAN is left alone. GOPACSRedispatchHandler exposes the asset id it was started for. The lifecycle test gains CREATE, DELETE, same-EAN UPDATE, changed-EAN UPDATE (enabled and disabled) and stop() cases for the poller; the two changed-EAN cases failed before this change.
Toggling redispatchEnabled through the asset editor had no effect until an attribute event or a service restart: processAssetChange only acted on the poller when the EAN changed, and an asset merge emits no attribute events. The UPDATE branch now reconciles the poller with the saved asset state: exactly one poller under the current EAN when redispatch is enabled, none otherwise. A poller already running under the current EAN is still left alone, for the same reason as before: its announcement bookkeeping is in memory only and a restart would re-record history. Replaces the stale-EAN-only helper from the previous commit. The lifecycle test gains the enable-on-save and disable-on-save cases, which failed before this change.
wborn
requested changes
Sep 16, 2026
wborn
left a comment
wborn
left a comment
Member
There was a problem hiding this comment.
This is an initial AI-assisted review.
The REST client cleanup, explicit deployment lifecycle, and service-stop cleanup are good improvements. One blocking concurrency issue remains in the scheduled-task teardown: an in-flight request can still schedule new work after undeploy() has cancelled and cleared the currently tracked futures. See the inline comment.
A request already inside processRawMessage could schedule work after undeploy() had cancelled and cleared the tracked futures. That task outlived the handler and later ran against a closed client. The endpoint is now undeployed first, so no further request can reach processRawMessage, and schedule() refuses under the same monitor the cancel loop holds. Either a task is tracked and then cancelled, or it is never scheduled at all. Also drops the trailing comments that the formatter had split across the schedule() calls and the statements after them. They named fixed 10s and 30s delays while both values come from configuration.
Both registries are touched from the persistence route, the attribute event subscription and the container stop thread. Stopping handlers by asset id and reconciling the redispatch poller iterate them with entrySet().removeIf while stop() walks them with forEach, so a concurrent put or remove on a HashMap could throw ConcurrentModificationException or corrupt the table.
Both registries are keyed by contracted EAN, and registering a new handler put straight over any entry already under that key. The displaced handler kept its JAX-RS endpoint and REST client open with no key left to stop it by, so two assets sharing an EAN leaked exactly what this branch set out to stop leaking. Stopping the current entry first covers every caller, including the redispatch reconcile, which could reach this case for a poller owned by another asset.
DELETE stopped by the EAN on the deleted entity while UPDATE stops by asset id. The entity carries the EAN as saved, which is not necessarily the key the running handler is registered under, so the same silent no-op that UPDATE had applied here. The asset-scoped stop for the poller is now a helper shared with the redispatch reconcile, which was doing the same removeIf inline.
GOPACSRedispatchHandler creates its client and then builds two proxies from it. If client.target(...) throws, no reference escapes the constructor, so stopPolling() can never run and the client is left open. Same guard the GOPACS handler already got.
Every save of the asset tore the handler down and built a new one, so a rename dropped the participant cache and cancelled any UFTP conversation waiting on a delayed response or FlexOffer. The redispatch poller was already spared for the same reason. The handler reads only the EAN, the realm and the asset id from the asset, everything else from container config, so leaving it alone when the EAN still maps to this asset is safe. CREATE and UPDATE now run the same reconcile, since creating an asset is the case where nothing is deployed yet. That drops the branch that duplicated the start calls, and with stopping done by asset id the EAN no longer has to be unwrapped before choosing what to do, so clearing the EAN on a save now stops the handler instead of being ignored. Also drops the keepEan parameter from the poller's asset-scoped stop. Deciding which poller to leave alone belongs in the reconcile, which now returns early, and the helper matches its GOPACS counterpart again.
Miggets7
changed the title
wborn
requested changes
Sep 17, 2026
wborn
left a comment
wborn
left a comment
Member
There was a problem hiding this comment.
The previous scheduled-task teardown race is fixed, and the latest reconciliation changes avoid unnecessarily rebuilding a handler when the EAN is unchanged.
One lifecycle issue remains in EmsOptimisationService: asset persistence changes can also generate attribute events, so the GOPACS and redispatch lifecycle can be processed independently by both paths. This can cause redundant teardown/recreation and, when the paths overlap, can lose ownership of a live handler. See the inline comment.
This review was AI-assisted.
A merge does raise attribute events. AssetStorageService.publishModificationEvents() publishes one per created attribute on CREATE, and on UPDATE one per added or modified attribute, or one per attribute outright when the save touched anything besides the attributes. So every save of a GOPACS asset reached this service twice: once through the persistence route, once through the attribute event subscription, which ran their own separate stop/start pairs. That undid the previous commit. Renaming the asset republishes an unchanged contractedEan, and the attribute path tore the handler down and built a new one anyway, losing the participant cache and every UFTP conversation waiting on a delayed response or FlexOffer. The two paths also run on different threads: PersistenceTopic is a seda endpoint with multipleConsumers, and the subscriber dispatch route hands events to its own executor. Interleaved, both stops could find nothing to stop and both puts land, leaving the first handler registered over by the second with its endpoint and client still open. Both paths now call the same reconcile, so whichever arrives second finds the handler already in its target state and leaves it alone, and the whole transition is held under a lifecycle lock so the two cannot interleave. Container start and stop take the same route, which drops the duplicated start calls in start(). The tests play back a save as the manager performs it, the persistence event followed by the attribute events raised from it, rather than the persistence event on its own.
schedule() was widened to protected and pendingTaskCount() added purely so GOPACSHandlerTest could reach them. Neither belongs in the production API: schedule() is called only from within the handler, and the pending count is internal bookkeeping no caller has any business reading. schedule() goes back to private and pendingTaskCount() is gone. The test reaches both reflectively, which it has to do because Groovy dispatches to the recording subclass and will not find a private member of the superclass.
save() already replayed the persistence event and its attribute events for an EAN change and a rename, but clearing the EAN and toggling redispatch were only tested through processAssetChange() directly. Both are handled by the same nonAttributeChange republish as a rename, so they are exactly the case the lifecycle-lock fix in the previous commit is meant to cover. save() now takes an optional oldRedispatchEnabled, alongside the existing oldEan, so a test can state the toggle's previous value when the republished attribute event needs one.
# Conflicts: # ems/src/main/java/org/openremote/extension/ems/manager/EmsOptimisationService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #97 (GOPACS half). The Distro Energy status attributes from the same issue land in #96.
Client and task leaks
GOPACSHandler.undeploy()now closes theResteasyClient. Every update of anEmsGOPACSAssetdid stop-then-start, so each edit leaked the previous client and its connection pool. Closing is safe with the shared executor:createClient(ExecutorService)goes throughResteasyClientBuilder.executorService, which leavesContainer.EXECUTORuntouched. Same fix as f878f18 forDistroEnergyHandler.Both GOPACS constructors close the client if
client.target(...)throws aftercreateClient, since no reference escapes a constructor that threw and the teardown method could never run.All scheduling goes through one
schedule()helper that records everyScheduledFuture. Before, only the futures fromprocessRawMessagewere tracked, so the power updates fromschedulePowerUpdatesurvivedundeploy(). Teardown takes the endpoint down first, then sets a flag and cancels under thescheduledFutureListmonitor, andschedule()checks that flag under the same monitor. A request already insideprocessRawMessagewhen teardown starts cannot queue work that outlives the handler.EmsOptimisationService.stop()now undeploysgopacsHandlerMap. It only handled the redispatch pollers and optimisation timers before.Both registries are
ConcurrentHashMap. They are read and written from the persistence route, the attribute event subscription and the container stop thread, and this branch iterates them while removing entries.Registering a handler under an EAN already in use stops the handler it displaces. Both maps are keyed by EAN and the put went straight over any live entry, leaving it with its endpoint and client open and no key to stop it by.
Deployment lifecycle
deploy()moved out of the constructor into a public no-arg method, called byEmsOptimisationServiceafter the handler is registered in its map. The JAX-RS resource routes straight toprocessRawMessage, so the handler was reachable from request threads before construction finished. Same shape asDistroEnergyHandler.deploy()andGOPACSRedispatchHandler.startPolling().CREATE and UPDATE now run one reconcile per handler, since creating an asset is the case where nothing is deployed yet:
processAssetChangestopped by the EAN on the updated entity, which carries the new value, so an EAN edit through the asset editor was a silent no-op stop: the handler under the old EAN kept its endpoint and client alive next to the new one. The attribute-event path already used the old value, but an asset merge emits no attribute events. Stopping now goes by asset id, on DELETE as well.redispatchEnabledtoggled through the asset editor takes effect, which previously needed an attribute event or a restart. A poller already running under the current EAN is left alone, because its announcement bookkeeping is in memory only and history entries are not deduplicated against the asset, so a restart would re-record every open announcement.Tests
GOPACSHandlerTestgains five cases: undeploy on a test-support handler is a no-op, undeploy cancels every pending future exactly once, completed futures are pruned on the next schedule call, and scheduling after undeploy reaches the executor neither directly nor through a processed FlexRequest.GOPACSHandlerHttpTestcallsdeploy()after constructing the handler.New
EmsOptimisationServiceGopacsTestcovers CREATE, DELETE by asset id, UPDATE with the same EAN, UPDATE with a changed EAN, UPDATE that clears the EAN,redispatchEnabledtoggled on save, displacing a handler registered under an EAN already in use, andstop()for both the GOPACS handler and the redispatch poller. It builds recording subclasses through the real constructors, with a fakeContainercarrying the OAuth config and a temp private-key file. Same pattern as the Distro Energy service test in #96. Each behaviour case fails without its fix../gradlew :ems:testagainst the local OR stack: 55 tests, 0 failures.