Fwd: [openvehicles/Open-Vehicle-Monitoring-System-3] f84837: Server V2 & V3: fix registration for notifications...
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? It seems that the better solution is to change OvmsServerV2::~OvmsServerV2 to set MyOvmsServerV2Reader=0 after deregistering (which is the original bug, imho)? Regards, Mark.
Begin forwarded message:
From: Michael Balzer <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
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> 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
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
Michael, Thanks for the clarification. That makes more sense. I do find the use of a static outside the class kind of kludgy/ugly. What if RegisterReader firstly iterated over the map to see if there was a free slot below m_nextreader? Or change the map to an array? Then we could simply keep the reader ID as a member variable of the class. Worth it, or not? Regards, Mark.
On 18 Nov 2019, at 5:35 PM, Michael Balzer <dexter@expeedo.de> wrote:
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 <https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3> Commit: f84837a2ce40d32769134d0bb5fe6dee09d842b3 https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/commit/f848... <https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/commit/f84837a2ce40d32769134d0bb5fe6dee09d842b3> 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 <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>
-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26 _______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
participants (2)
-
Mark Webb-Johnson -
Michael Balzer