Parallel enables occurring (when SIM PIN enabled)

[Aleksander Morgado]

On Wed, Apr 26, 2017 at 6:23 PM, Dan Williams <dcbw at redhat.com> wrote:
>> > > > I've encountered a problem when using MM with Network Manager,
>> > > > when
>> > > > the SIM PIN is enabled - in short it seems that MM is able to
>> > > > hit a
>> > > > state where two 'enable' operations are happening in parallel,
>> > > > one
>> > > > from unlocking the SIM and one from starting the 'simple
>> > > > connect'.
>> > > > The detail and discussion so far has been over at http://www.ma
>> > > > il-a
>> > > > rchive.com/networkmanager-list at gnome.org/msg27008.html , but it
>> > > > does seem to be an issue that essentially lies in MM.
>> > >
>> > > Could you open a new bugreport in bugzilla and attach the
>> > > relevant
>> > > logs there as well? I believe we should track this.
>> > > https://bugs.freedesktop.org/enter_bug.cgi?product=ModemManager
>> >
>> > He originated this thread on the NM list (not sure if you saw it
>> > there)
>> > and I diagnosed what was going on from an internal
>> > perspective.  It's a
>> > race between the state change to ENABLING and the polkit auth
>> > stuff.
>> > We need to check right after the polkit auth that our state hasn't
>> > changed already due to another request, and then do the right thing
>> > (either return an error or allow the specific op to proceed).
>>
>> May have missed the email in the NM list. I saw the patch attached to
>> the bugreport now, and my only concern was related to why a final
>> state > ENABLED (e.g. registered or connected) would be treated as an
>> error during the Enable() request.
>
> Patch was more for confirmation/debugging than intended for commit.  We
> can plug this particular hole with patches like that, but probably want
> to figure out a more general strategy.  That said, I think this the
> most likely case we'd encounter.

Also probably the easiest case to handle; queuing multiple Enable()
requests so that only the first request does the logic and the
remaining ones just wait for the first one to finish, is something we
can definitely do. This could also apply to the disable phase, or the
simple.disconnect, or the bearer.connect/bearer.disconnect phases.

Whenever there are specific settings requested for a given action,
though, (e.g. simple.connect) we should force to have a single
operation running always, and error out right away without waiting for
any final state (unless we also want to compare the settings and queue
the new request if the settings are the same one as the original one).

-- 
Aleksander
https://aleksander.es