[vlc-devel] [PATCH 5/7] playlist: fix deadlock on destruction while preparser adds items to playlist

[Filip Roséen]

Hi Rémi,

On 2017-03-29 22:01, Rémi Denis-Courmont wrote:

> Le keskiviikkona 29. maaliskuuta 2017, 20.45.59 EEST Filip Roséen a écrit :
> > > > It fixes the issue described in ticket [#18151][1], which you closed as
> > > > a
> > > > duplicate of [#17652][2]. I would be interested to see know how #18151
> > > > is
> > > > not fixed, as I did extensive testing and a lot of proof-reading to
> > > > inspect the relevant paths taken.
> > > 
> > > I would be interested to know how a lock inversion can be fixed while not
> > > settling for one lock order or the other, nor eliminating the code paths
> > > leading to either orders.
> > 
> > As stated, you are the one who closed `#18151` as a duplicate of
> > `#17652`; the ticket description for the former in detail describe
> > what lock is being taken, and why it deadlocks.
> 
> It is not strictly a duplicate but more of a specific instance of it. And as 
> ALREADY NOTED BEFORE, this is just removing one code path leading to it.
>
> I did warn Thomas that this was a playlist problem and not a preparser/
> prefetcher problem quite a while back. If the whole point of this patch series 
> was to fix this bug, then it seems that you missed the point and unfortunately 
> wasted your time.

I do not believe that I have wasted my time, as such it has not been a
waste of time (as what constitutes as "waste of time" is highly
subjective); you are of course entitled to your own opinion.

Once again, I am very interested in knowledge related to how the
issue described by `#18151` is reproducible after the relevant set of
patches.

> > > That it can´t be reproduced with the Qt, maybe. However that it is fixed
> > > sounds logically impossible.
> > 
> > This was extensively tested with various setups, with and without
> > *Qt* (as well as with other interfaces just for good measure). This
> > has little to nothing to do with the interface used as it was very
> > easily reproducible "without" (`-Idummy`) any interface at all.. as
> > the problem lies in how clean-up is being done (which previously
> > raced, as described in `#18151`).
> 
> I stopped caring about "extensively tested" when it comes to race conditions a 
> long time ago. Even more so when the bug seems still present from looking at 
> the code.

Extensive testing is a valuable tool which at least gives some
indication of whether a problem can occur in practice or not, that
together with code analysation is how I attack bugs like this.

> How do you LOGICALLY explain the bug being fixed?

By looking at what can be executing at a given time, and what paths
that can (and are allowed) to be taken. Looking at what
synchronizations are acquired and released for a given entity leading
up to the point of destruction. That's how I prove that the deadlock
that occurs while an entity being preparsed tries to add items to the
playlist while we are trying to destroy the playlist is fixed.

In other words: `#18151`.

> 
> -- 
> 雷米‧德尼-库尔蒙
> https://www.remlab.net/
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170329/766d0a8e/attachment.html>