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

Submitted by Frediano Ziglio on July 11, 2019, 1 p.m.

Details

Message ID 20190711130054.17867-7-fziglio@redhat.com
State Accepted
Commit ca641a5b92df55f17b711b8ed7b7c5ec6b8249f9
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 11, 2019, 1 p.m.
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.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/usb-backend.c        | 7 +++++++
 src/usb-backend.h        | 1 +
 src/usb-device-manager.c | 4 ++++
 3 files changed, 12 insertions(+)

Patch hide | download patch | download mbox

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;
     }

Comments


> 
> 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."

> > Signed-off-by: Frediano Ziglio <fziglio@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