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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

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?

Bart