[libcamera-devel] [PATCH v3 18/30] py: unittests: Fix test_select()

[Tomi Valkeinen]

On 27/05/2022 22:09, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:
>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to
> 
> s/current/currently/
> 
>> see that cm.get_ready_requests() returns something. This is not always
>> the case, as there may be two eventfd events queued, and the first call
>> to cm.get_ready_requests() returns all the requests, and thus the second
>> call returns none.

The above is actually not quite true. There are no "eventfd events", 
just a value, and if that value is > 0, the eventfd is readable.

So what happens here is that the user calls read_event(), which clear 
that value. Then, before the user calls get_ready_requests(), there's a 
request-complete event, and the bindings increase the value. And now, 
when the user calls get_ready_requests(), it clears all the requests, 
but the eventfd value is 1 and thus the select on the eventfd will 
trigger immediately again, but there are no requests in the list.

>> Remove the self.assertTrue(len(ready_reqs) > 0) assert.
> 
> This convinces me even more that there's room for improvement :-) It
> seems a bit confusing for applications.

I'm open to suggestions =).

I like the current method as it's very straightforward, just one mutex 
which is only kept when adding a Request to the gReqList, or swap()'ing 
the gReqList to a local var. And the read_event() and 
get_ready_requests() should allow the caller to handle the event loop as 
he wishes, both blocking and non-blocking.

Now, we do always call read_event() and get_ready_requests() together in 
our .py scripts. So perhaps we should merge them, and add new separate 
functions if someone ever needs them. However, it won't change the 
problem solved in this patch in any way.

We could also change the merged function so that the mutex is kept while 
read() is called and the gReqList is swap()'d. This would solve the 
problem fixed in this patch. However, it would mean that we'd possibly 
be blocked in read() while keeping the mutex and that would cause a 
deadlock.

Anyway, my opinion is that it's not an issue if there's an event from 
libcamera but get_ready_requests() doesn't return anything.

  Tomi