Mark, Am 18.11.19 um 02:37 schrieb Mark Webb-Johnson:
I don’t think this fix is correct. Seeking clarification of the thinking behind it.
Currently, MyOvmsServerV2Reader is initialised to 0. Then, OvmsServerV2::OvmsServerV2 creates the reader (and sets MyOvmsServerV2Reader to it) if it is zero. Then OvmsServerV2::~OvmsServerV2 deregisters (clears) the reader, but does not set MyOvmsServerV2Reader to 0.
The change is to OvmsServerV2::OvmsServerV2 to register the reader (without setting MyOvmsServerV2Reader) if MyOvmsServerV2Reader is 0.
This seems to leak one reader every time the OvmsServerV2 is stopped/started, as without storing in MyOvmsServerV2Reader, the old reader will never be cleared upon deregistering. Or am I missing something?
You miss the MyOvmsServerV2Reader being given to the second registration call to be reused. The current implementation of reader registration is allocating one of 32 available bits for a new reader. The bit allocation is permanent, the servers accordingly only allocate their ids once. But the reader entry in the list of active notification readers isn't permanent. To be able to re-register for a previously allocated bit number, I introduced the second RegisterReader() method some time ago. That was for the webserver context -- registering a new id on every new websocket connection would run out of ids very soon. It became apparent (from a user report on the weekend) that both v2 & v3 servers did not register as readers on a second start, so have the same issue. You can test the previous implementation and the fix by watching "notify status".
It seems that the better solution is to change OvmsServerV2::~OvmsServerV2 to set MyOvmsServerV2Reader=0 after deregistering (which is the original bug, imho)?
That would actually leak reader bits due to the permanent allocation. It would have been an option to add a bit mask for the use/free status so you don't need to permanently store and reuse the allocated reader id. IIRC I decided against that because the v2&v3 servers had that scheme of permanent storage. Thinking about it now, that would be a more clean, more standard way option for the notify API, maybe we should do that. Regards, Michael
Regards, Mark.
Begin forwarded message:
*From: *Michael Balzer <noreply@github.com <mailto:noreply@github.com>> *Subject: **[openvehicles/Open-Vehicle-Monitoring-System-3] f84837: Server V2 & V3: fix registration for notifications...* *Date: *17 November 2019 at 11:22:46 PM HKT *To: *mark@webb-johnson.net <mailto:mark@webb-johnson.net>
Branch: refs/heads/master Home: https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3 Commit: f84837a2ce40d32769134d0bb5fe6dee09d842b3 https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/commit/f848... Author: Michael Balzer <balzer@expeedo.de <mailto:balzer@expeedo.de>> Date: 2019-11-17 (Sun, 17 Nov 2019)
Changed paths: M vehicle/OVMS.V3/components/ovms_server_v2/src/ovms_server_v2.cpp M vehicle/OVMS.V3/components/ovms_server_v3/src/ovms_server_v3.cpp
Log Message: ----------- Server V2 & V3: fix registration for notifications on restart
_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26