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

[Laurent Pinchart]

Hi Tomi,

On Mon, May 30, 2022 at 11:36:52AM +0300, Tomi Valkeinen wrote:
> On 30/05/2022 11:25, Laurent Pinchart wrote:
> > 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() ?
> 
> read_event() blocks until there is an "event". In our current .py files 
> this is fine, because in all the cases we either 1) call read_event() & 
> get_ready_requests() because select indicates that there is an event, or 
> 2) we want the read_event() to block if there's no event.
> 
> I think the only case where merging read_event() and 
> get_ready_requests() would cause a problem is if you want to do polling. 
> At the moment you can just call get_ready_requests() and it never blocks 
> and returns any requests that have been completed.
> 
> Then again, even with a merged function the above could be accomplished 
> if the user changes the eventfd to non-blocking mode in the application 
> code. With that change, read_event() never blocks, although at the 
> moment it throws an exception if read() fails (because at the moment 
> read() should never fail). We should probably then change it to return a 
> ret val instead.
> 
> But I don't really know if polling is a use case we're interested in 
> supporting. Even blocking read feels a bit pointless. I think in all but 
> the most simple examples you want to properly wait on the fd, instead of 
> polling or blocking. Especially as it's trivial to do with Python.

Agreed :-)

-- 
Regards,

Laurent Pinchart