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

[Filip Roséen]

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

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

Opinion acknowledged.

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

Given that you say that my assumptions are wrong, the code should be
easy to break, or there should be a very easy way to demonstrate that
my assumptions about *condition-variables* are wrong.

I am happy to take a conforming implementation of the specification as
an example, so if you could be kind to link me the source of such I
would happily read it.

If my assumptions are wrong, I will be the first to say that I have
been wrong, but I cannot do that without knowing what it is that is
wrong, and why it is 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.

We have a very different set of opinion in terms of what is considered
to be *"racy"* and *"pointless"*. If you feel like you have a better
way of solving the describe set of problems together with the
functionality now added (which I believe is a nice addition); please
state so, or submit a patches.

If you are on the other hand stating that there is no problem, I fail
to see what your opinion really is about the matter.

> > 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.
 
/F
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170329/126c19c7/attachment-0001.html>