Allow sleep() to accept non-integer values

3 years ago by vorismi3@fel.cvut.cz — view source

unread

Hi everyone,

I am the author of https://github.com/php/php-src/pull/5961 , please
provide feedback.

All details should be in the description, also, please advise if we can
consider it as a small change not requiring RFC as Nikita proposed in
his comment.

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

3 years ago by Rowan Tommins — view source

unread

On Tue, 11 Aug 2020 at 08:31, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

I am the author of https://github.com/php/php-src/pull/5961 , please
provide feedback.

This idea makes a lot of sense to me as a user (I'll leave comments on the
implementation to those with more C experience).

I'm pretty sure I've accidentally written things like "sleep(1.5)" in the
past, and think there is a strong argument for changing something here:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled), but the input is casted to 0 and thus producing
unexpected behaviour if the user is not aware of the current method
prototype.

Unless there are problems with the implementation, this seems like a
straight-forward win.

Regards,

Rowan Tommins
[IMSoP]

3 years ago by bjorn.x.larsson@telia.com — view source

unread

Den 2020-08-11 kl. 10:53, skrev Rowan Tommins:

On Tue, 11 Aug 2020 at 08:31, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

I am the author of https://github.com/php/php-src/pull/5961 , please
provide feedback.

This idea makes a lot of sense to me as a user (I'll leave comments on the
implementation to those with more C experience).

I'm pretty sure I've accidentally written things like "sleep(1.5)" in the
past, and think there is a strong argument for changing something here:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled), but the input is casted to 0 and thus producing
unexpected behaviour if the user is not aware of the current method
prototype.

Unless there are problems with the implementation, this seems like a
straight-forward win.

Regards,

Given this unexpected behaviour, one could almost see it as a bug. I think
it's worth considering if this also should be fixed in 8.0 or even
earlier ;-)
So good to hear the RM view on this.

r//Björn L

3 years ago by vorismi3@fel.cvut.cz — view source

unread

Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available
for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Den 2020-08-11 kl. 10:53, skrev Rowan Tommins:

On Tue, 11 Aug 2020 at 08:31, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

I am the author of https://github.com/php/php-src/pull/5961 , please
provide feedback.

This idea makes a lot of sense to me as a user (I'll leave comments on the
implementation to those with more C experience).

I'm pretty sure I've accidentally written things like "sleep(1.5)" in the
past, and think there is a strong argument for changing something here:

Another reason is that sleep(0.1); is silently accepted now (even with strict types enabled), but the input is casted to 0 and thus producing
unexpected behaviour if the user is not aware of the current method
prototype.

Unless there are problems with the implementation, this seems like a
straight-forward win.

Regards,

Given this unexpected behaviour, one could almost see it as a bug. I
think
it's worth considering if this also should be fixed in 8.0 or even
earlier ;-)
So good to hear the RM view on this.

r//Björn L

3 years ago by G. P. B. — view source

unread

On Thu, 20 Aug 2020 at 14:15, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available

for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or
even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

3 years ago by G. P. B. — view source

unread

Apologies for the double email, my client did something funcky.

On Thu, 20 Aug 2020 at 14:15, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available

for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or
even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Again, I personally don't understand why this could bypass the RFC process,
as multiple people have already, me included, voiced their disagreement
with this change.

Secondly this change introduces another inconsistency, why can sleep accept
a float but not usleep?

If there is indeed a need for being able to specify a sleep in milliseconds
I'd prefer the introduction of a
msleep function then this change.

Best regards

George P. Banyard

3 years ago by vorismi3@fel.cvut.cz — view source

unread

Again, I personally don't understand why this could bypass the RFC process, as multiple people have already, me included, voiced their disagreement with this change.

This was proposed by Nikita Popov in his comment

Secondly this change introduces another inconsistency, why can sleep accept a float but not usleep?

Nanosleep and microsleep functions are basically 1:1 of the underlaying
implementation. The updated sleep() is however now never worse than the
best sleep function available, thus we can use it also for
time_nanosleep and usleep php function and accept float. I will
implement it.

If there is indeed a need for being able to specify a sleep in milliseconds I'd prefer the introduction of a msleep function then this change.

The issue I solve is sleep function that accepts seconds should accept
floating point values as time is continous value.

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Apologies for the double email, my client did something funcky.

On Thu, 20 Aug 2020 at 14:22, G. P. B. george.banyard@gmail.com wrote:

On Thu, 20 Aug 2020 at 14:15, Michael Voříšek - ČVUT FEL vorismi3@fel.cvut.cz wrote: Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available

for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Again, I personally don't understand why this could bypass the RFC
process,
as multiple people have already, me included, voiced their disagreement
with this change.

Secondly this change introduces another inconsistency, why can sleep
accept a float but not usleep?

If there is indeed a need for being able to specify a sleep in
milliseconds I'd prefer the introduction of a
msleep function then this change.

Best regards

George P. Banyard

3 years ago by Gabriel Caruso — view source

unread

On Thu, 20 Aug 2020 at 14:55, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

Again, I personally don't understand why this could bypass the RFC
process, as multiple people have already, me included, voiced their
disagreement with this change.

This was proposed by Nikita Popov in his comment

Nikita is just one of the members. If other members want an RFC, we need it.

Also, the PR has a couple of :-1: votes as well, which indicates that an
RFC is necessary.

Secondly this change introduces another inconsistency, why can sleep
accept a float but not usleep?

Nanosleep and microsleep functions are basically 1:1 of the underlaying
implementation. The updated sleep() is however now never worse than the
best sleep function available, thus we can use it also for
time_nanosleep and usleep php function and accept float. I will
implement it.

If there is indeed a need for being able to specify a sleep in
milliseconds I'd prefer the introduction of a msleep function then this
change.

The issue I solve is sleep function that accepts seconds should accept
floating point values as time is continous value.

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Apologies for the double email, my client did something funcky.

On Thu, 20 Aug 2020 at 14:22, G. P. B. george.banyard@gmail.com
wrote:

On Thu, 20 Aug 2020 at 14:15, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote: Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available

for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or
even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Again, I personally don't understand why this could bypass the RFC
process,
as multiple people have already, me included, voiced their disagreement
with this change.

Secondly this change introduces another inconsistency, why can sleep
accept a float but not usleep?

If there is indeed a need for being able to specify a sleep in
milliseconds I'd prefer the introduction of a
msleep function then this change.

Best regards

George P. Banyard

3 years ago by kocsismate90@gmail.com — view source

unread

Again, I personally don't understand why this could bypass the RFC
process, as multiple people have already, me included, voiced their
disagreement with this change.

This was proposed by Nikita Popov in his comment

Nikita is just one of the members. If other members want an RFC, we need
it.

Also, the PR has a couple of :-1: votes as well, which indicates that an
RFC is necessary.

Just for clarity, Nikita didn't propose such thing that the feature can
skip the RFC process. The exact words he used is the following:

This does not necessarily need an RFC, but needs a discussion on the

mailing list at least,

as the reception here seems somewhat negative. Default target version is

8.1 now, though

exceptions can be made for small features.

All this means is:

The discussion on the ML is a must
IPHP 8.1 is the default target

Regards:
Máté

3 years ago by Gabriel Caruso — view source

unread

On Thu, 20 Aug 2020 at 14:16, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

Hi everyone,

thank you for your comments, based on them, I fixed these:

usleep is now used as a fallback as well, if interrupted, remaining
time is measured using microtime, so return value is always available

for BC, if not interrupted, return value remains to be 0 (integer
zero)

Now, the sleep() function should be really universal, cross platform and
I would say also the prefered way to sleep.

The implementaion is here https://github.com/php/php-src/pull/5961/files

Please comment on Github directly if you have any feedback left.

I thinkit's worth considering if this also should be fixed in 8.0 or
even earlier ;-)So good to hear the RM view on this.

Sara, are you ok to include this in PHP 8.0 and do you require a RFC for
it?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Den 2020-08-11 kl. 10:53, skrev Rowan Tommins:

On Tue, 11 Aug 2020 at 08:31, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

I am the author of https://github.com/php/php-src/pull/5961 , please
provide feedback.

This idea makes a lot of sense to me as a user (I'll leave comments on
the
implementation to those with more C experience).

I'm pretty sure I've accidentally written things like "sleep(1.5)" in the
past, and think there is a strong argument for changing something here:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled), but the input is casted to 0 and thus producing
unexpected behaviour if the user is not aware of the current method
prototype.

Unless there are problems with the implementation, this seems like a
straight-forward win.

Regards,

Given this unexpected behaviour, one could almost see it as a bug. I
think
it's worth considering if this also should be fixed in 8.0 or even
earlier ;-)
So good to hear the RM view on this.

r//Björn L

I don't know about Sara's opinion here, but mine, as PHP 8.0's other
Release Manager, this change requires an RFC
and it should target PHP 8.1. PHP 8.0 has been feature frozen for a while
now.

3 years ago by Christoph M. Becker — view source

unread

Unless there are problems with the implementation, this seems like a
straight-forward win.

Not necessarily a problem, but it has to be considered that POSIX
mandates that nanosleep() shall fail, if "the rqtp argument specified a
nanosecond value […] greater than or equal to 1000 million."[1]
Analogous for usleep() which has been removed from the POSIX standard.

[1]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/nanosleep.html

--
Christoph M. Becker

3 years ago by Dan Ackroyd — view source

unread

Michael Voříšek wrote:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled),

That appears to not be true: https://3v4l.org/7YbqX

Rowan wrote:

Unless there are problems with the implementation, this seems like a
straight-forward win.

From the PR.

Implemented using nanosleep which is not guaranteed to be available everywhere.

Please just use usleep if you need more accuracy than seconds.

Having a function that behaves differently based on different
platforms is a bad idea.

Changing a function to have surprising behaviour just to avoid using a
different function, that is already available, is a really bad
tradeoff.

cheers
Dan
Ack

3 years ago by vorismi3@fel.cvut.cz — view source

unread

Another reason is that sleep(0.1); is silently accepted now (even with strict types enabled),

That appears to not be true: https://3v4l.org/7YbqX

corrected, should be "without strict types enabled" -
https://3v4l.org/A2olN

"even with strict type enabled" statement in BC section remains valid

Having a function that behaves differently based on different platforms is a bad idea.

I will implement fallback to usleep. I have not noticed any issues with
usleep availability. Is there any known platform without nanosleep
neither usleep support?

With kind regards / Mit freundlichen Grüßen / S přátelským pozdravem,

Michael Voříšek

Michael Voříšek wrote:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled),

That appears to not be true: https://3v4l.org/7YbqX

Rowan wrote:

Unless there are problems with the implementation, this seems like a
straight-forward win.

From the PR.

Implemented using nanosleep which is not guaranteed to be available everywhere.

Please just use usleep if you need more accuracy than seconds.

Having a function that behaves differently based on different
platforms is a bad idea.

Changing a function to have surprising behaviour just to avoid using a
different function, that is already available, is a really bad
tradeoff.

cheers
Dan
Ack

3 years ago by G. P. B. — view source

unread

On Tue, 11 Aug 2020 at 13:03, Michael Voříšek - ČVUT FEL <
vorismi3@fel.cvut.cz> wrote:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled),

That appears to not be true: https://3v4l.org/7YbqX

corrected, should be "without strict types enabled" -
https://3v4l.org/A2olN

"even with strict type enabled" statement in BC section remains valid

That's a normal type juggling behaviour of PHP to silently truncate float
to integers.
Maybe something that we should change by emitting a E_WARNING but that
would need an RFC.
Therefore I don't see why sleep should get special treatment.

I also echo Dan's concerns and I find this change all in all negative.
Now we need to deal with IEEE 754 floating point semantics and converting
"manually" to an integer range
to pass it to functions which might not be available on the platform and
then still end up using sleep(0) on the C level.

I'd rather see the failure condition changed to reject 0 or add a
E_NOTICE/E_WARNING if 0 is passed as this has questionable semantics.

Best regards

George P. Banyard

3 years ago by Dan Ackroyd — view source

unread

Björn Larsson wrote:

Given this unexpected behaviour, one could almost see it as a bug.

This isn't a suddenly noticed new bug. That code has worked like that
since the sleep function was committed twenty-two years ago or for
five years since the release of PHP 7 and the weak/strict RFC
continued the "if you don't want float -> int conversions, use strict
mode" choice.

There are quite a few functions in PHP that take int parameters, that
have this behaviour.

These could all be 'fixed' by people using strict mode, and you could
make a good argument for making strict mode the default, but it's
probably a bit late in the 8.0 release cycle for that, and it might
be a little contentious.

I agree that the PHP standard library is not as good as it could be,
but tweaking it piece-by-piece is not a good way of fixing it.

Doing it like that would lead to subtle BC breaks that are not obvious.

G.P.B. wrote:

I'd rather see the failure condition changed to reject 0 or add a
E_NOTICE/E_WARNING if 0 is passed as this has questionable semantics.

sleep(0) is a useful thing to do in some circumstances. It has the
same effect as Thread.yield() in Java.

Or in English, it allows a process to say "Hello OS scheduler, I don't
want to sleep right now, but if there are any other threads that are
waiting for their turn, now is a great time to suspend my execution,
as I'm not in the middle of anything, otherwise I'll keep processing."

Again, yes we need a better way of evolving the core libraries. But
tweaking it on a function by function basis is not a good way of doing
that.

cheers
Dan
Ack

3 years ago by Matteo Beccati — view source

unread

Hi Michael,

Changing a function to have surprising behaviour just to avoid using a
different function, that is already available, is a really bad
tradeoff.

I agree.

function mysleep(float $s): void {
usleep($s * 1000000);
}

You're welcome ;-)

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

3 years ago by twosee — view source

unread

2020年8月11日下午6:39，Dan Ackroyd Danack@basereality.com 写道：

Michael Voříšek wrote:

Another reason is that sleep(0.1); is silently accepted now (even with
strict types enabled),

That appears to not be true: https://3v4l.org/7YbqX

Rowan wrote:

Unless there are problems with the implementation, this seems like a
straight-forward win.

From the PR.

Implemented using nanosleep which is not guaranteed to be available everywhere.

Please just use usleep if you need more accuracy than seconds.

Having a function that behaves differently based on different
platforms is a bad idea.

Changing a function to have surprising behaviour just to avoid using a
different function, that is already available, is a really bad
tradeoff.

cheers
Dan
Ack

--

To unsubscribe, visit: https://www.php.net/unsub.php

I would rather have a new function like this (actually I am using it on my project):

function msleep(int $milli_seconds) { }

int is always a better choice than float, otherwise, we have to do dirty conversions and more checks on it
And usually, we don't need high precision, millisecond sleep is enough, e.g. the timeout feature provided by epoll, it is the reason why many async libraries provide timeout feature in milliseconds.

Regards,
Twosee