Follow-up to #96. Three unrelated items in the EMS extension, grouped because they are all small and touch the same handlers.
1. GOPACSHandler never closes its REST client
GOPACSHandler stores a ResteasyClient as a field (GOPACSHandler.java:126, assigned at :219) and undeploy() (:326-332) cancels the scheduled futures and undeploys the web service, but never closes it.
EmsOptimisationService.processAssetChange does stop-then-start on every UPDATE of an EmsGOPACSAsset, so each edit of the asset leaves the previous client and its connection pool allocated. The client has already made requests by then, so it holds real sockets.
This is the same defect fixed for DistroEnergyHandler in #96 (f878f18). The fix is the same shape:
public void undeploy() {
for (ScheduledFuture<?> scheduledFuture : scheduledFutureList) {
scheduledFuture.cancel(true);
}
scheduledFutureList.clear();
client.close();
webService.undeploy(getDeploymentName(contractedEAN));
}
Closing is safe with the shared executor. WebTargetBuilder.createClient(ExecutorService) goes through ResteasyClientBuilder.executorService(ExecutorService), which sets cleanupExecutor to false, so Container.EXECUTOR is untouched. GOPACSRedispatchHandler.stopPolling() already does this.
Worth checking the same constructor window #96 closed: anything between createClient and the end of the constructor that can throw leaks a client, because no reference escapes a constructor that threw and undeploy() can never run. In GOPACSHandler the client.target(...) calls at :222-224 are the candidates.
2. Two redispatch attributes are declared and displayed but never written
The Redispatch — Bid panel in asset-types.json shows three attributes. Two of them can never hold a value:
| Attribute |
Declared |
Written |
Result |
redispatchSuggestedPower |
EmsGOPACSAsset.java:153, READ_ONLY |
nowhere |
always null |
redispatchSuggestedVolume |
EmsGOPACSAsset.java:163, READ_ONLY |
nowhere |
always null |
redispatchBidPrice |
EmsGOPACSAsset.java:168 |
nowhere |
operator must enter it blind |
Both suggested values are marked READ_ONLY, so the operator cannot fill them either. The panel renders two permanently empty fields.
The section comment above them says // --- Redispatch bid attributes (auto-calculated, operator-overridable) ---. The auto-calculation was never implemented. redispatchBidPrice is read at GOPACSRedispatchHandler.java:570 when placing a bid, so the intent appears to have been that the handler pre-fills a suggested price and power from the announcement, and the operator overrides if they want.
redispatchSuggestedPower also carries STORE_DATA_POINTS, HAS_PREDICTED_DATA_POINTS and DATA_POINTS_MAX_AGE_DAYS=7, so it is set up for charting that will never have data.
Decide which way to go and do it:
- implement the calculation and write all three on each announcement, next to the existing writes in
processAnnouncement (GOPACSRedispatchHandler.java:424-456), or
- drop the two suggested attributes and remove them from the panel.
Leaving them declared and displayed but unpopulated is the one option that should not stay.
3. General improvements
Audit every declared attribute for a writer. The two above were found by diffing the AttributeDescriptor constants in EmsGOPACSAsset against the sendAttributeEvent call sites. Worth doing the same sweep across the other EMS asset types, and worth a test that fails when a READ_ONLY attribute has no writer, since that combination is always a bug.
EmsDistroEnergyAsset has no observability at all. It declares only portfolio, and its panel config shows only that. After #96 the handler decides per run how many market days to submit and where the forecast horizon ends, and none of that is visible on the asset. redispatchLastPoll is the precedent worth copying:
lastSubmission (TIMESTAMP, READ_ONLY) so a stalled handler is visible without reading logs
daysSubmitted (NUMBER, READ_ONLY) which is the forecast horizon in days, currently only a FINE log line
Constructor-publishes-this pattern. #96 fixed this in DistroEnergyHandler by moving scheduling into a deploy() method called after construction (afc6d9f). GOPACSHandler schedules from its constructor too and is worth the same treatment, which also makes it constructible in a test without an executor.
GOPACSHandler has no Level.WARNING budget discipline. Not urgent, but the DST-collapse warning added in #96 fires per position, so on the fall-back day it can emit several hundred lines per portfolio. Root cause is openremote/openremote#3292 upstream; a rate limit or a once-per-day summary would make it readable in the meantime.
Follow-up to #96. Three unrelated items in the EMS extension, grouped because they are all small and touch the same handlers.
1.
GOPACSHandlernever closes its REST clientGOPACSHandlerstores aResteasyClientas a field (GOPACSHandler.java:126, assigned at:219) andundeploy()(:326-332) cancels the scheduled futures and undeploys the web service, but never closes it.EmsOptimisationService.processAssetChangedoes stop-then-start on everyUPDATEof anEmsGOPACSAsset, so each edit of the asset leaves the previous client and its connection pool allocated. The client has already made requests by then, so it holds real sockets.This is the same defect fixed for
DistroEnergyHandlerin #96 (f878f18). The fix is the same shape:Closing is safe with the shared executor.
WebTargetBuilder.createClient(ExecutorService)goes throughResteasyClientBuilder.executorService(ExecutorService), which setscleanupExecutorto false, soContainer.EXECUTORis untouched.GOPACSRedispatchHandler.stopPolling()already does this.Worth checking the same constructor window #96 closed: anything between
createClientand the end of the constructor that can throw leaks a client, because no reference escapes a constructor that threw andundeploy()can never run. InGOPACSHandlertheclient.target(...)calls at:222-224are the candidates.2. Two redispatch attributes are declared and displayed but never written
The Redispatch — Bid panel in
asset-types.jsonshows three attributes. Two of them can never hold a value:redispatchSuggestedPowerEmsGOPACSAsset.java:153,READ_ONLYredispatchSuggestedVolumeEmsGOPACSAsset.java:163,READ_ONLYredispatchBidPriceEmsGOPACSAsset.java:168Both suggested values are marked
READ_ONLY, so the operator cannot fill them either. The panel renders two permanently empty fields.The section comment above them says
// --- Redispatch bid attributes (auto-calculated, operator-overridable) ---. The auto-calculation was never implemented.redispatchBidPriceis read atGOPACSRedispatchHandler.java:570when placing a bid, so the intent appears to have been that the handler pre-fills a suggested price and power from the announcement, and the operator overrides if they want.redispatchSuggestedPoweralso carriesSTORE_DATA_POINTS,HAS_PREDICTED_DATA_POINTSandDATA_POINTS_MAX_AGE_DAYS=7, so it is set up for charting that will never have data.Decide which way to go and do it:
processAnnouncement(GOPACSRedispatchHandler.java:424-456), orLeaving them declared and displayed but unpopulated is the one option that should not stay.
3. General improvements
Audit every declared attribute for a writer. The two above were found by diffing the
AttributeDescriptorconstants inEmsGOPACSAssetagainst thesendAttributeEventcall sites. Worth doing the same sweep across the other EMS asset types, and worth a test that fails when aREAD_ONLYattribute has no writer, since that combination is always a bug.EmsDistroEnergyAssethas no observability at all. It declares onlyportfolio, and its panel config shows only that. After #96 the handler decides per run how many market days to submit and where the forecast horizon ends, and none of that is visible on the asset.redispatchLastPollis the precedent worth copying:lastSubmission(TIMESTAMP,READ_ONLY) so a stalled handler is visible without reading logsdaysSubmitted(NUMBER,READ_ONLY) which is the forecast horizon in days, currently only a FINE log lineConstructor-publishes-
thispattern. #96 fixed this inDistroEnergyHandlerby moving scheduling into adeploy()method called after construction (afc6d9f).GOPACSHandlerschedules from its constructor too and is worth the same treatment, which also makes it constructible in a test without an executor.GOPACSHandlerhas noLevel.WARNINGbudget discipline. Not urgent, but the DST-collapse warning added in #96 fires per position, so on the fall-back day it can emit several hundred lines per portfolio. Root cause is openremote/openremote#3292 upstream; a rate limit or a once-per-day summary would make it readable in the meantime.