[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility

[Rémi Denis-Courmont]

Le keskiviikkona 29. maaliskuuta 2017, 20.50.50 EEST Filip Roséen a écrit :
> > > The thread is per definition not waiting for the conditiona variable
> > > at that time, so I cannot see how it would wake itself up.
> > 
> > You are making unwarranted assumptions about condition variables here.
> 
> Assumptions based on specifications, I would however be extremely
> interested on implimentations where this "assumption" does not hold
> (mostly because a issue should definitely be filed based on such
> finding).

> > Been there done that.
> 
> Previous paragraph.

There is not much point in discussing if we cannot agree on facts such as the 
meaning of the specification.

Both my experience and my understanding of CV tells me that this patch is 
factually wrong.

> > > > Also it´s highly questionable that more than one thread should ever
> > > > wait
> > > > on
> > > > the worker. Normally, we probe the item, we set the flag and release
> > > > the
> > > > item. And move onto the next item, or exit.
> > > 
> > > I am taking the safe route as this path is also invoked from users of
> > > *libvlc*. We do not control who waits for what, and given that it is
> > > not documented that a single waiter is max (by intention given the
> > > implementaiton) the code is written as it is.
> > 
> > You can never have more than one thread waiting in deletion path. Anything
> > else would lead to multiple frees.
> 
> I have nver said that there should be two entities waiting for the
> preparser deletion, I am talking about cancellations of preparser
> requests.

Which should never be waited for because it´s racy and pointless.

> It also seems that I am forced to add, even though I thought
> it was rather obvious from the implementation and comments, that there
> is nothing stating that you cannot remove everything from the
> preparser queue twice, or cancel a single request twice, or perhaps
> cancel a group of requests by using the same id for all of them,
> 
> `background_worker_Destroy` implies `background_worker_Cancel` (with
> the `id` argument being NULL to remove everything), but not the other
> way around.


-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/