gh-125997: ensure that time.sleep(0) is not delayed on non-Windows platforms

[picnixz]

This is a suggestion for fixing the issue when the timeout is 0. For other timeouts, this does not change the current behaviour (I'm tempted removing the do-while loop as we should never have an issue, but I'm not familar with the monotonic C implementation so I left it as is).

• Issue: time.sleep(0) regression in 3.11 onwards #125997

[rruuaanng]

@picnixz Unfortunately, we should seriously review :(. It seems that an assertion failed somewhere.

Assertion failed: (errno == 0), function pysleep, file timemodule.c, line 2220.
Fatal Python error: Aborted

Thread 0x0000000173bbb000 (most recent call first):
  File "/Users/admin/actions-runner/_work/cpython/cpython/Lib/subprocess.py", line 1918 in _execute_child
...

[picnixz]

Ah I think I know what happens. When we raise an OSError from errno we don't clear the errno. So if you chain calls, you need to just ignore them. I'm removing the assertion cc @ZeroIntensity

[rruuaanng]

If pytime is running on multiple cores, I recommend using _LIBC_REENTRANT, which will make your errno independent in each thread.

raw defined

#  if !defined _LIBC || defined _LIBC_REENTRANT
/* When using threads, errno is a per-thread value.  */
#   define errno (*__errno_location ())
#  endif
# endif /* !__ASSEMBLER__ */
#endif /* _ERRNO_H */

[picnixz]

This wouldn't solve the issue (also because _LIBC_REENTRANT is a private macro and something you supply when configuring libc). By the way, errno is thread-safe (see https://linux.die.net/man/3/errno). The issue is simply that we can raise an OSError from errno, which doesn't clear errno, and do something else afterwards. I will keep the current behaviour as it was done beforehand.

[ZeroIntensity]

TIL! Should we assert !PyErr_Occurred() instead?

[picnixz]

TIL! Should we assert !PyErr_Occurred() instead?

Not sure. It's not really an issue and the function is internal and only for time.sleep(). It's only used once (it's just that time.sleep() calls this function). Though it wouldn't hurt (if this assertion fails, then there is a separate issue)

[ZeroIntensity]

I think we should. There's no real cost to adding it, and it would be helpful for debugging in the rare case that something goes wrong.

[hauntsaninja]

Linking #28111 for other reviewers

I'd vote we can get rid of the do-while, I think it would only matter if monotonic is not monotonic

I'd also vote we don't backport. This is basically a performance optimisation (which we don't typically backport) for a regression that has only been reported once in two years and that has an easy workaround.

[hauntsaninja]

nit: I think we can get rid of err, seems like an artifact of the version of this code that has a bunch of #ifdef

[picnixz]

I'd vote we can get rid of the do-while, I think it would only matter if monotonic is not monotonic

I wanted to be sure that "PyTime_Monotonic() != -1" means that "monotonic > deadline" but if this is guaranteed, then I'd be happy to simplify the code.

I'd also vote we don't backport. This is basically a performance optimisation (which we don't typically backport) for a regression that has only been reported once in two years and that has an easy workaround.

I'm also fine with this (I just treated it as a regular bug fix but we can treat it as a performance feature). The workaround you're talking about is to use poll.poll(0) directly, right?

[hauntsaninja]

Yes, using poll.poll directly or maybe using os.sched_yield (on 3.11.1 and newer)