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.

Sign up for GitHub

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

Jump to bottom

tests/*server.pl: flush output before executing subprocess #7530

Closed
wants to merge 2 commits into from
Closed

tests/*server.pl: flush output before executing subprocess #7530

wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Aug 3, 2021

Also avoid shell processes staying around by using exec. This may solve the random Windows CI issues.

@mback2k mback2k added the tests label Aug 3, 2021
@mback2k mback2k self-assigned this Aug 3, 2021
@mback2k mback2k mentioned this pull request Aug 7, 2021
Closed
mback2k added a commit to mback2k/curl that referenced this pull request Aug 15, 2021
@mback2k
tests/*server.pl: flush output before executing subprocess
218c649 
Also avoid shell processes staying around by using exec.

Closes curl#7530
@mback2k mback2k marked this pull request as ready for review August 15, 2021 11:28
@mback2k mback2k requested review from bagder and jay August 15, 2021 11:28
@mback2k
Copy link
Member Author

mback2k commented Aug 15, 2021

After a total of 5 CI runs without any AppVeyor or Azure test failing in this PR, I am quite sure this will finally fix their flakiness.

jay
jay approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message needs an explanation as to why this change is necessary, and any references where it is discussed.

mback2k added a commit to mback2k/curl that referenced this pull request Aug 17, 2021
@mback2k
tests/*server.pl: flush output before executing subprocess
70f11be 
Also avoid shell processes staying around by using exec.
This is necessary to avoid output data being buffering
inside the process chain of Perl, Bash/Shell and our
test server binaries. On non-Windows systems the exec
will also make the subprocess replace the intermediate
shell, but on Windows it will at least bind the processes
together since there is no real fork or exec available.

See: https://cygwin.com/cygwin-ug-net/highlights.html
and: https://docs.microsoft.com/cpp/c-runtime-library/exec-wexec-functions

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro
Closes curl#7530
@mback2k mback2k requested a review from jay August 17, 2021 10:29
@mback2k
Copy link
Member Author

mback2k commented Aug 17, 2021

@jay thanks, please take another look at the updated commit message.

mback2k added a commit to mback2k/curl that referenced this pull request Aug 17, 2021
@mback2k
tests/*server.pl: flush output before executing subprocess
37cbd11 
Also avoid shell processes staying around by using exec.
This is necessary to avoid output data being buffering
inside the process chain of Perl, Bash/Shell and our
test server binaries. On non-Windows systems the exec
will also make the subprocess replace the intermediate
shell, but on Windows it will at least bind the processes
together since there is no real fork or exec available.

See: https://cygwin.com/cygwin-ug-net/highlights.html
and: https://docs.microsoft.com/cpp/c-runtime-library/exec-wexec-functions

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro
Closes curl#7530
@mback2k
tests/*server.pl: flush output before executing subprocess
d6a9aba 
Also avoid shell processes staying around by using exec.
This is necessary to avoid output data being buffering
inside the process chain of Perl, Bash/Shell and our
test server binaries. On non-Windows systems the exec
will also make the subprocess replace the intermediate
shell, but on Windows it will at least bind the processes
together since there is no real fork or exec available.

See: https://cygwin.com/cygwin-ug-net/highlights.html
and: https://docs.microsoft.com/cpp/c-runtime-library/exec-wexec-functions

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro
Closes curl#7530
@mback2k
tests/runtests.pl: use default post command delay of 100ms
15e0f50 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.
@mback2k
Copy link
Member Author

mback2k commented Aug 17, 2021

Unfortunately even this PR is experiencing some flakiness again, although it seems reduced. That is why I added another commit for testing purposes.

@mback2k
Copy link
Member Author

mback2k commented Aug 17, 2021

@jay @bagder if you are fine with the first commit (content and message), I would probably go ahead and merge that already as this PR and then move the remaining flakiness testing to a new PR.

@bagder
Copy link
Member

bagder commented Aug 18, 2021

Works for me!

@jay
Copy link
Member

jay commented Aug 18, 2021

I think it needs a reference to the issue(s) where the random CI fails are discussed, so it can be understood why it is necessary to make this change to avoid the buffering. In other words the issue that you are fixing. I'm not really opposed to this but I think that there may be some underlying issue here that we just aren't aware of that is causing these problems. That said if it is an improvement then 👍

@mback2k
Copy link
Member Author

mback2k commented Aug 18, 2021 •
edited

Thanks! @jay it is hard to link to a single issue or instance of this flakiness since we have been experiencing that on Azure and AppVeyor since months now. You can see it in the test analytics of Azure quite well:
https://dev.azure.com/daniel0244/curl/_test/analytics?definitionId=1&contextType=build

Filter like this:
image
image

You will see the test failures spread quite evenly over a lot of tests (test 2100 was recently fixed by you):
image

@mback2k mback2k closed this in 5b1c2dd Aug 18, 2021
mback2k added a commit to mback2k/curl that referenced this pull request Oct 8, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
7c2e18b 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
@mback2k mback2k mentioned this pull request Oct 8, 2021
Closed
mback2k added a commit to mback2k/curl that referenced this pull request Oct 12, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
4caaecb 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
mback2k added a commit to mback2k/curl that referenced this pull request Oct 13, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
da23eb1 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
mback2k added a commit to mback2k/curl that referenced this pull request Oct 27, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
e3a0db6 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
mback2k added a commit to mback2k/curl that referenced this pull request Nov 1, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
a358780 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
mback2k added a commit to mback2k/curl that referenced this pull request Nov 1, 2021
@mback2k
tests/runtests.pl: use default post command delay of 100ms
a6ebe28 
Although flushing the output buffers reduced the flakiness
of the curl test suite on Windows already, some tests were
still randomly failing. This is another attempt to fix it.

Follow up to curl#7530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

@jay jay Awaiting requested review from jay

Assignees

@mback2k mback2k

Labels
tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mback2k @bagder @jay