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

[Laurent Pinchart]

Hi Tomi,

On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote:
> On 27/05/2022 22:09, Laurent Pinchart wrote:
> > 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.

I've expressed myself incorrectly. This is totally fine, what I'd like
to see removed is the read_event() function. What happens if you call
read_event) in get_ready_requests() ?

-- 
Regards,

Laurent Pinchart