Re: [libgpiod] Python bindings don't allow to wait on events indefinitely

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, May 23, 2023 at 12:03:14PM +0200, Bartosz Golaszewski wrote:
> On Fri, May 19, 2023 at 4:56 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Fri, May 19, 2023 at 04:32:32PM +0200, Nicolas Frattaroli wrote:
> > > On Freitag, 19. Mai 2023 07:17:27 CEST Kent Gibson wrote:
> > > > On Thu, May 11, 2023 at 10:28:34PM +0200, Nicolas Frattaroli wrote:
> > > > > Hello,
> > > > >
> > > > > in libgpiod 1.6.x, Line.event_wait's codepath had no path where ts
> > > > > as passed to ppoll could ever be NULL. This means waiting indefinitely
> > > > > was impossible.
> > > > >
> > > > > I thought hey, maybe the new Python bindings in libgpiod 2.x fixed this,
> > > > > but no, it has made it worse by explicitly setting timeout to 0 seconds
> > > > > if it's None[1]. Obviously, this behaviour can't be changed now, because
> > > > > people depend on this API to return immediately now with None as the
> > > > > parameter, and changing it to wait indefinitely would no doubt break
> > > > > actual programs.
> > > > >
> > > > > So I'm left wondering if there's a particular reason users of these
> > > > > bindings shouldn't wait on events indefinitely or if that same mistake
> > > > > was just made twice in a row.
> > > > >
> > > > > Is there some way the API could be enhanced to support waiting for
> > > > > events indefinitely without having to slap a While True with
> > > > > an arbitrarily high timeout around every single invocation?
> > > > >
> > >
> > > Hello Kent,
> > >
> > > > That does sound like a bug to me, but the rest of your mail isn't worth
> > > > responding to.
> > >
> > > I'm not quite sure what you mean. Was my tone this off? I apologise if
> > > you took my displeasure with libgpiod's bindings as a personal attack,
> > > it wasn't intended as such.
> > >
> >
> > Not a personal attack on me, but offensive none the less.
> > Assume you are the code owner and see how it parses to you.
> >
> > > > A more productive approach could be to submit a patch that describes the
> > > > problem and suggests a fix, say:
> > > >
> > > >  def poll_fd(fd: int, timeout: Optional[Union[timedelta, float]] = None) -> bool:
> > > > -    if timeout is None:
> > > > -        timeout = 0.0
> > > > -
> > > >
> > > > and see where that goes.
> > >
> > > That would go nowhere, as this makes the API behave differently for current
> > > users calling the function without an argument, as I've mentioned.
> > >
> >
> > By that logic any bug that has visible effects cannot be fixed - in case
> > some user depends on those effects.
> > And all bugs have visible effects.
> >
> > The function docs don't specify the behaviour when None is passed -
> > despite None being the default.
> >
> > > One solution would be to pass float("inf") and then check for that, this
> > > wouldn't break the API, merely extend it, but I'm not sure how good of
> > > an idea that is to do if someone uses an older libgpiod that doesn't
> > > have an explicit check for inf. I don't even know what passing inf does
> > > right now, but it's probably worth looking into.
> > >
> >
> > Please no.
> >
> > I would expect the API should stick to standard Python conventions,
> > similar to select itself - " When the timeout argument is omitted the
> > function blocks until at least one file descriptor is ready."
> > so wait_edge_event() should block, as should wait_edge_event(None)
> >
> > > > to submit a patch
> > >
> > > Move the project back to a git forge and I will.
> > >
> >
> > And there you go being confrontational again. GFY.
> >
> > Read the README - you submit patches to this list.
> >
> > And not my project, so not my call on where it is hosted.
> > I'm just trying to help you out here, but I'm left wondering why.
> >
> 
> I wouldn't call it a bug, more like unusual behavior as far as Python APIs go.
> 
> I never noticed it before because I typically use the file descriptor
> directly and pass it to the polling function of choice.
> 

And I never reviewed the implementation - but from the higher level
functions (wait_edge_events()) I expected that None would block, as that
is the standard Python convention.

> In any case - the easiest way to fix this would be allowing the use of
> a negative number to indicate that we should wait forever. Any issue
> with that?
> 

As above, None makes more sense to me in a Python context.
I expect that anyone calling wait_edge_events() would expect that to
block, right?  It does say "wait".

Anyone currently using it will find that it is a NOP, so I doubt there
are many of those.  And the documentation needs to updated the clarify
what None does, either way.

IIWU, I'd rather fix this correctly now, rather than be explaining the
negative means blocking to Python users until the end of time.

Cheers,
Kent.