[vlc-devel] [PATCH] Deprecate vlc_clone_detach()

[Romain Vimont]

On Sun, Sep 06, 2020 at 03:15:05PM +0200, Romain Vimont wrote:
> Hi,
> 
> On Sun, Sep 06, 2020 at 03:32:27PM +0300, Rémi Denis-Courmont wrote:
> > The documention outright lies about LibVLC waiting to make this functon
> > safe. There are no ways to use this function safely other than linking
> > LibVLC statically into the application, which is not *generally*
> > practical.
> 
> I have mixed feelings about this.
> 
> I'm wondering if it would not just be better to just document that
> unloading the libvlc library is undefined behavior. Moreover, VLC
> dlopen()s on module_need() but does not dlclose() on module_unneed()
> anyway, because it is unsafe.
> 
> Detached threads are convenient and they release their resources as soon
> as possible. IMO it would be too bad to forbid them only for being able
> to (theoretically) unload libvlc.
> 
> I think I agree with this comment:
> 
> > Unloading is fundamentally unsafe unless a library is specifically
> > written to be safe against unloading, so the default should be
> > "nodelete" with an explicit option required to make unloading
> > possible.
> 
> https://stackoverflow.com/a/51146634/1987178
> 
> Regards

Given that:
 - plugins are expected to be safely unloadable [1],
 - vlc_clone_detach() can always be replaced,
 - unloading libvlc is used in practice,

then patch LGTM.

Regards

[1]: https://mailman.videolan.org/pipermail/vlc-devel/2020-September/137159.html

> 
> > ---
> >  src/libvlc.h       |  2 +-
> >  src/posix/thread.c | 23 +++++++----------------
> >  2 files changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/libvlc.h b/src/libvlc.h
> > index d2d112aa56..884af326a3 100644
> > --- a/src/libvlc.h
> > +++ b/src/libvlc.h
> > @@ -44,7 +44,7 @@ void vlc_CPU_dump(vlc_object_t *);
> >   * Threads subsystem
> >   */
> >  
> > -/* This cannot be used as is from plugins yet: */
> > +VLC_DEPRECATED
> >  int vlc_clone_detach (vlc_thread_t *, void *(*)(void *), void *, int);
> >  
> >  int vlc_set_priority( vlc_thread_t, int );
> > diff --git a/src/posix/thread.c b/src/posix/thread.c
> > index d8edc13d53..33f6da47bc 100644
> > --- a/src/posix/thread.c
> > +++ b/src/posix/thread.c
> > @@ -229,27 +229,18 @@ void vlc_join(vlc_thread_t th, void **result)
> >  
> >  /**
> >   * Creates and starts new detached thread.
> > + *
> >   * A detached thread cannot be joined. Its resources will be automatically
> >   * released whenever the thread exits (in particular, its call stack will be
> >   * reclaimed).
> >   *
> > - * Detached thread are particularly useful when some work needs to be done
> > - * asynchronously, that is likely to be completed much earlier than the thread
> > - * can practically be joined. In this case, thread detach can spare memory.
> > - *
> > - * A detached thread may be cancelled, so as to expedite its termination.
> > - * Be extremely careful if you do this: while a normal joinable thread can
> > - * safely be cancelled after it has already exited, cancelling an already
> > - * exited detached thread is undefined: The thread handle would is destroyed
> > - * immediately when the detached thread exits. So you need to ensure that the
> > - * detached thread is still running before cancellation is attempted.
> > - *
> > - * @warning Care must be taken that any resources used by the detached thread
> > - * remains valid until the thread completes.
> > + * \warning
> > + * Detached thread are intrinsically unsafe to use, as there are no ways to
> > + * know whence the thread has terminated cleanly and its resources can be
> > + * reclaimed (in particular, the executable code run by the thread).
> >   *
> > - * @note A detached thread must eventually exit just like another other
> > - * thread. In practice, LibVLC will wait for detached threads to exit before
> > - * it unloads the plugins.
> > + * \bug
> > + * This function should be removed. Do not use this function in new code.
> >   *
> >   * @param th [OUT] pointer to hold the thread handle, or NULL
> >   * @param entry entry point for the thread
> > -- 
> > 2.28.0
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel