[Ovmsdev] Metrics & Notifications: thread safety

[Stephen Casner]

Using a separate task is particularly appealing for the stack
requirements consideration.  Would the extra delay be a problem for
any of the recipients?

                                                        -- Steve

On Wed, 23 Jan 2019, Michael Balzer wrote:

> Jakob,
>
> bool, int and float are 32 bit values, so reads and writes are atomic on ESP32. A read-modify-write cycle would not be atomic, but the metrics API does not
> provide that kind of operation.
>
> The effect of SetModified(false) is not to reset the modified bits, it just updates m_defined & m_stale. So this does not cancel out a concurrent
> SetModified(true). However, the current implementation does not guarantee NotifyModified() will happen at most once for the same change.
>
> A solution could be to decouple NotifyModified() from SetModified(), possibly moving it to a dedicated task as well. I've been a little bit discomforted since
> the beginning with metrics listeners being called within the modifying context. Neither the metric modifier knows about the listener time and stack
> requirements, nor does the listener know about the restrictions of the current context. For example, the transmission of the metric change via network can run
> within the CAN RX context. Even the creation of a possibly complex notification can be coupled directly to a metrics change.
>
> So maybe decoupling the callback execution is the next step here.
>
> Regards,
> Michael
>
>
> Am 23.01.19 um 16:28 schrieb Jakob Löw:
> > Hey,
> >
> > OvmsMetricBool, -Int and -Float don't seem to use a mutex, but afaik
> > there is no garantuee for them to be atomic. A simple fix would be to
> > make m_value a std::atomic<float> etc. or use a mutex just as with
> > OvmsMetricString.
> > Furthermore there might be a small race condition when two tasks modify
> > a metric. Assuming the following base situation:
> >
> > OvmsMetricString dummy("dummy");
> > dummy.SetValue("a");
> >
> > // some other code...
> >
> > dummy.SetValue("a");
> > // now between ovms_metrics.cpp line 793 and 794 the thread is interrupted and this happens:
> >
> > dummy.SetValue("b");
> > // the value is modified, all change listener are called but none resets the modified flag.
> >
> > // now execution continues with line 794 of dummy.SetValue("a"); from above
> > // which essentially does SetModified(false);
> > // but in fact the current value is "b" so it was modified.
> >
> > This also applies to all other metric types, as they all seem to call
> > SetModified after changing the value.
> > The easy fix would be to put the SetModified into the mutex lock, but
> > that would result in a deadlock when one of the change listeners calls
> > SetValue.
> > I don't see an better way to avoid that race condition though :(
> >
> > - Jakob
> >
> > On Tue, 2019-01-22 at 15:49 +0100, Michael Balzer wrote:
> >> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/comm
> >> it/8003fc0da759d5d33682c54cbf38840823477fbc
> >>
> >> Brain surgery. A second pair of eyes on this would be good.
> >>
> >> Regards,
> >> Michael
> >>
> >> _______________________________________________
> >> 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
>
>