Code review comment for lp:~thomas-voss/location-service/fix-nm-race

Revision history for this message
Thomas Voß (thomas-voss) wrote :

> Hmm the locking is a bit spaghetti now; I can see why lock needs to be passed
> around to unlock before dispatching the updates from the on_foo() callbacks,
> but it does feel a bit weird to pass the lock down a callback and unlock it
> from there.
>

The one case is technically not a callback but a functor passed for evaluation. In all other cases, the lock is created within the callback and handed down to the actual function. I do prefer this API style to naming functions *_locked(...), which is tedious and error prone.

> I guess the only way would be to queue the added/removed wifis/cells into a
> queue and trigger the updates once the lock is released, but that's pretty
> much as weird.
>
> Anyway, tested, and seems to help with the otherwise failing test case.
>
> Note that: a) I wasn't able to reproduce on mako + vivid, so couldn't confirm
> this fixed anything; b) it did help reproducible breakage on krillin from
> earlier revisions, but I did manage to break HERE on one boot.
>

> So generally, this helps; I suspect there's still a race for a case.
>
> I did witness tech=cell switching to tech=wifi on krillin when coming out of
> airplane mode, which was nice!

Thanks.

« Back to merge proposal