Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testStdinPipe fails intermittently on s390x #309

Closed
bryceharrington opened this issue Aug 4, 2020 · 3 comments
Closed

testStdinPipe fails intermittently on s390x #309

bryceharrington opened this issue Aug 4, 2020 · 3 comments
Assignees
Labels
Milestone

Comments

@bryceharrington
Copy link

In Ubuntu, on s390x (only), we've been noticing occasional failures with CLITest::testStdinPipe in phpmyadmin-sql-parser version 4.6.1. This test runs the php binary six times. Out of five different CI runs:

A: data set #2 and #5 failed
B: data set #2 and #3 failed
C: data set #1 and #3 failed
D: data set #3 and #4 failed
E: data sets #0, #1, #2, #3, #4, #5 all failed

https://autopkgtest.ubuntu.com/packages/phpmyadmin-sql-parser/groovy/s390x

In the cases where the CI passed, we weren't doing anything special, it seems to have just randomly passed.

I unfortunately don't have access to the s390x machine so am unable to investigate this directly, just via the CI system. The php cli and test suite pass properly on my (non-s390x) hardware. I'm hoping you may have some insights to help us make the test more deterministic?

@ibennetch
Copy link
Member

ibennetch commented Aug 4, 2020 via email

@williamdes williamdes self-assigned this Aug 4, 2020
@williamdes
Copy link
Member

Thank you for reporting this here and on launchpad https://bugs.launchpad.net/phpmyadmin-sql-parser/+bug/1890341

Assigned to myself because I manage the packaging part

sergiodj pushed a commit to sergiodj/sql-parser that referenced this issue Aug 26, 2020
The idiom used in the readStdin function from src/Utils/CLI.php is not
entirely error-prone: marking stdin as non-blocking and then reading
from it can lead to racy scenarios where we are not able to catch
what's being fed to the program.  This can be seen very frequently on
s390x, where a loop like this:

  $ while echo "invalid query" | /usr/bin/php7.4 /path/to//highlight-query; do sleep 1; done;

will lead to failures once on every few runs (as can be seen in the
description of issue phpmyadmin#309).

This commit implements the more robust method of using stream_select
to wait for stdin to become available, but resorting to a sensible
timeout value (0.2 seconds) which will prevent the blocking from
happening.

I tested this patch extensively on s390x and amd64, and noticed that
the non-determinism is gone.

Signed-off-by: Sergio Durigan Junior <[email protected]>
sergiodj pushed a commit to sergiodj/sql-parser that referenced this issue Aug 27, 2020
The idiom used in the readStdin function from src/Utils/CLI.php is not
entirely error-prone: marking stdin as non-blocking and then reading
from it can lead to racy scenarios where we are not able to catch
what's being fed to the program.  This can be seen very frequently on
s390x, where a loop like this:

  $ while echo "invalid query" | /usr/bin/php7.4 /path/to//highlight-query; do sleep 1; done;

will lead to failures once on every few runs (as can be seen in the
description of issue phpmyadmin#309).

This commit implements the more robust method which uses stream_select
to wait for stdin to become available, but resorts to a sensible
timeout value (0.2 seconds) which will prevent the blocking from
happening.

I tested this patch extensively on s390x and amd64, and noticed that
the non-determinism is gone.

Signed-off-by: Sergio Durigan Junior <[email protected]>
sergiodj pushed a commit to sergiodj/sql-parser that referenced this issue Aug 27, 2020
The idiom used in the readStdin function from src/Utils/CLI.php is not
entirely error-prone: marking stdin as non-blocking and then reading
from it can lead to racy scenarios where we are not able to catch
what's being fed to the program.  This can be seen very frequently on
s390x, where a loop like this:

  $ while echo "invalid query" | /usr/bin/php7.4 /path/to//highlight-query; do sleep 1; done;

will lead to failures once on every few runs (as can be seen in the
description of issue phpmyadmin#309).

This commit implements the more robust method which uses stream_select
to wait for stdin to become available, but resorts to a sensible
timeout value (0.2 seconds) which will prevent the blocking from
happening.

I tested this patch extensively on s390x and amd64, and noticed that
the non-determinism is gone.

Signed-off-by: Sergio Durigan Junior <[email protected]>
sergiodj added a commit to sergiodj/sql-parser that referenced this issue Aug 27, 2020
The idiom used in the readStdin function from src/Utils/CLI.php is not
entirely error-prone: marking stdin as non-blocking and then reading
from it can lead to racy scenarios where we are not able to catch
what's being fed to the program.  This can be seen very frequently on
s390x, where a loop like this:

  $ while echo "invalid query" | /usr/bin/php7.4 /path/to//highlight-query; do sleep 1; done;

will lead to failures once on every few runs (as can be seen in the
description of issue phpmyadmin#309).

This commit implements the more robust method which uses stream_select
to wait for stdin to become available, but resorts to a sensible
timeout value (0.2 seconds) which will prevent the blocking from
happening.

I tested this patch extensively on s390x and amd64, and noticed that
the non-determinism is gone.

Signed-off-by: Sergio Durigan Junior <[email protected]>
@williamdes williamdes added this to the 4.6.2 milestone Aug 27, 2020
@williamdes williamdes added the bug label Aug 27, 2020
williamdes added a commit to sergiodj/sql-parser that referenced this issue Aug 27, 2020
williamdes added a commit that referenced this issue Aug 27, 2020
Pull-request: #312
Fixes: #309

Signed-off-by: William Desportes <[email protected]>
williamdes added a commit that referenced this issue Aug 27, 2020
Signed-off-by: William Desportes <[email protected]>
@williamdes
Copy link
Member

Fixed by @sergiodj
Will be in the next 4.x and 5.x releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants