[Ovmsdev] Fwd: [openvehicles/Open-Vehicle-Monitoring-System-3] f84837: Server V2 & V3: fix registration for notifications...

[Michael Balzer]

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 at github.com <mailto:noreply at 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 at webb-johnson.net <mailto:mark at 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/f84837a2ce40d32769134d0bb5fe6dee09d842b3
>>  Author: Michael Balzer <balzer at expeedo.de <mailto:balzer at 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 at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20191118/489da3ea/attachment.htm>