[Spice-devel] [PATCH spice-gtk 7/7] usb-device-manager: Last chance to avoid deadlock handling libusb events

[Victor Toso]

Hi,

On Thu, Jul 11, 2019 at 09:42:02AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Thu, Jul 11, 2019 at 02:00:54PM +0100, Frediano Ziglio wrote:
> > > Attempt to better interrupt event handling loop.
> > > If the thread handling events is stuck waiting events or handling an
> > > event try to interrupt before joining the thread.
> > 
> > Do you have a UI stuck or something without this patch?
> > 
> 
> Good question. Never happened, however:
> - I had some experience in the past with libusb and multi-threading
>   and I know this call is useful;
> - for Unix (but not Windows) there's some lines above the comment
>         /* Force termination of the event thread even if there were some
>          * mismatched spice_usb_device_manager_{start,stop}_event_listening
>          * calls. Otherwise, the usb event thread will be leaked, and will
>          * try to use the libusb context we destroy in finalize(), which would
>          * cause a crash */
>    so calling that function on all (Unix and Windows) will help too.
> 
> 
> Maybe adding in commit message:
> 
> "For Unix code in spice_usb_device_manager_dispose will try to
> force some thread exit but this is not done for Windows
> so calling libusb_interrupt_event_handler will help.
> Code is not in a hot path so won't change the execution time."

Sounds reasonable to me, with that
Acked-by: Victor Toso <victortoso at redhat.com>

> 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/usb-backend.c        | 7 +++++++
> > >  src/usb-backend.h        | 1 +
> > >  src/usb-device-manager.c | 4 ++++
> > >  3 files changed, 12 insertions(+)
> > > 
> > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > index 48a62cd1..a2c502da 100644
> > > --- a/src/usb-backend.c
> > > +++ b/src/usb-backend.c
> > > @@ -230,6 +230,13 @@ gboolean
> > > spice_usb_backend_handle_events(SpiceUsbBackend *be)
> > >      return ok;
> > >  }
> > >  
> > > +void spice_usb_backend_interrupt_event_handler(SpiceUsbBackend *be)
> > > +{
> > > +    if (be->libusb_context) {
> > > +        libusb_interrupt_event_handler(be->libusb_context);
> > > +    }
> > > +}
> > > +
> > >  static int LIBUSB_CALL hotplug_callback(libusb_context *ctx,
> > >                                          libusb_device *device,
> > >                                          libusb_hotplug_event event,
> > > diff --git a/src/usb-backend.h b/src/usb-backend.h
> > > index 9f2a97a6..cbb73c22 100644
> > > --- a/src/usb-backend.h
> > > +++ b/src/usb-backend.h
> > > @@ -65,6 +65,7 @@ after it finishes list processing
> > >  SpiceUsbBackendDevice **spice_usb_backend_get_device_list(SpiceUsbBackend
> > >  *backend);
> > >  void spice_usb_backend_free_device_list(SpiceUsbBackendDevice **devlist);
> > >  gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be);
> > > +void spice_usb_backend_interrupt_event_handler(SpiceUsbBackend *be);
> > >  gboolean spice_usb_backend_register_hotplug(SpiceUsbBackend *be,
> > >                                              void *user_data,
> > >                                              usb_hot_plug_callback proc);
> > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > index 4960667e..0d12432f 100644
> > > --- a/src/usb-device-manager.c
> > > +++ b/src/usb-device-manager.c
> > > @@ -328,6 +328,8 @@ static void spice_usb_device_manager_dispose(GObject
> > > *gobject)
> > >  #endif
> > >      if (priv->event_thread) {
> > >          g_warn_if_fail(g_atomic_int_get(&priv->event_thread_run) ==
> > >          FALSE);
> > > +        g_atomic_int_set(&priv->event_thread_run, FALSE);
> > > +        spice_usb_backend_interrupt_event_handler(priv->context);
> > >          g_thread_join(priv->event_thread);
> > >          priv->event_thread = NULL;
> > >      }
> > > @@ -988,6 +990,8 @@ gboolean
> > > spice_usb_device_manager_start_event_listening(
> > >         libusb_handle_events call in the thread won't exit until the
> > >         libusb_close call for the device is made from usbredirhost_close.
> > >         */
> > >      if (priv->event_thread) {
> > > +        g_atomic_int_set(&priv->event_thread_run, FALSE);
> > > +        spice_usb_backend_interrupt_event_handler(priv->context);
> > >           g_thread_join(priv->event_thread);
> > >           priv->event_thread = NULL;
> > >      }
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190711/884dde57/attachment.sig>