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

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

Closed
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 force-pushed the flush-testserver-outputs branch from 81ac86c to 02439a5 Aug 7, 2021
mback2k added a commit to mback2k/curl that referenced this issue Aug 15, 2021
Also avoid shell processes staying around by using exec.

Closes curl#7530
@mback2k mback2k force-pushed the flush-testserver-outputs branch from 02439a5 to 218c649 Aug 15, 2021
@mback2k mback2k marked this pull request as ready for review Aug 15, 2021
@mback2k mback2k requested review from bagder and jay Aug 15, 2021
@mback2k
Copy link
Member Author

@mback2k 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.

bagder
bagder approved these changes Aug 15, 2021
jay
jay approved these changes Aug 16, 2021
Copy link
Member

@jay jay left a comment

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 issue Aug 17, 2021
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 force-pushed the flush-testserver-outputs branch from 218c649 to 70f11be Aug 17, 2021
@mback2k mback2k requested a review from jay Aug 17, 2021
@mback2k
Copy link
Member Author

@mback2k 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 issue Aug 17, 2021
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 force-pushed the flush-testserver-outputs branch from 70f11be to 37cbd11 Aug 17, 2021
mback2k added 2 commits Aug 17, 2021
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
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 mback2k force-pushed the flush-testserver-outputs branch from 37cbd11 to 15e0f50 Aug 17, 2021
@mback2k
Copy link
Member Author

@mback2k 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 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 bagder commented Aug 18, 2021

Works for me!

@jay
Copy link
Member

@jay 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 mback2k commented Aug 18, 2021

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants