[vlc-devel] [PATCH] Avoid deadlock on UDP stop

[Romain Vimont]

Le jeudi 22 mai 2014 à 12:42 +0800, Rémi Denis-Courmont a écrit :
> Le 2014-05-21 21:02, Romain Vimont a écrit :
> > - incomplete: on what condition do you set b_eof?
> 
> You've answered that question yourself previously, when the return
> is NULL.

I think you refer to this message:
https://mailman.videolan.org/pipermail/vlc-devel/2014-May/098120.html

I said the main point of the patch is to return NULL once we know the
EOF condition is met (b_finishing in that case). Returning NULL is the
*consequence* of the EOF detection, so it cannot be the *cause*.

However, by "when the return is NULL", I think you mean "when
block_FifoGet() returns NULL". Unfortunately, there is no equivalence
between A and B:
 A. block_FifoGet() returns NULL
 B. we reached EOF (i.e. there is no way that a new block can be added
    to the fifo queue anymore)

(A ==> B) is true:
  block_FifoGet() cannot return NULL if block_FifoWake() is not called.
  As block_FifoWake() is called only at the end of ThreadRead(),
  block_FifoGet() can return NULL only when we reached EOF. □

However, (B ==> A) is false:
  By definition, we consider that we reach EOF once ThreadRead() cannot
  put more blocks into to fifo queue anymore, i.e. once it calls
  block_FifoWake(). The semantic of block_FifoWake() is to make
  exactly one call to block_FifoGet() nonblocking. During that call, the
  fifo may not be empty (i.e. p_fifo->p_first != NULL). In that case, it
  does not return NULL (and p_fifo->b_force_wake will remain unset). □

Therefore, setting b_eof when block_FifoGet() returns NULL is not
sufficient, it would not fix the deadlock.

> > - insufficient: how could that resolve the deadlock?
> 
> Your patch does not solve the deadlock so much as it works around
> it. I am not even sure that the root cause of the problem has been
> properly identified.

My understanding is that the semantic of block_FifoWake() does not match
what ThreadRead() expects: the command "unblock the current/next call to
block_FifoGet()" is too weak, because when the fifo queue is not empty,
the next call would not have been blocking anyway, so block_FifoWake()
have absolutely no effect.

Instead, the command should be either "do not retrieve any block
anymore" (what this patch implements), or "retrieve pending blocks, but
stop (do not block) once the queue is empty".

I consider this as a root cause in itself. But maybe there is a
super-cause I am not aware of. Or maybe the mismatch between what
ThreadRead() expects and the semantic of block_FifoWake() comes from a
wrong block_FifoWake() semantic; but then, what about its use in
input_DecoderWait()?

--
®om