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

tool and tests: force flush of all buffers at end of program #8516

Closed
wants to merge 1 commit into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Feb 26, 2022

On Windows data can be lost in buffers in case of abnormal program
termination, especially in process chains as seen due to flaky tests.
Therefore flushing all buffers manually should avoid this data loss.

In the curl tool we play the safe game by only flushing write buffers,
but in the testsuite where we manage all buffers, we flush everything.

This should drastically reduce the CI and testsuite flakiness.

Supersedes #7833 and #6064

@mback2k
Copy link
Member Author

mback2k commented Feb 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Feb 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k mback2k force-pushed the tests-sync-fs branch 2 times, most recently from a0d401d to de87e31 Compare March 5, 2022 18:46
@mback2k mback2k added the Windows Windows-specific label Mar 5, 2022
@mback2k
Copy link
Member Author

mback2k commented Mar 5, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

On Windows data can be lost in buffers in case of abnormal program
termination, especially in process chains as seen due to flaky tests.
Therefore flushing all buffers manually should avoid this data loss.

In the curl tool we play the safe game by only flushing write buffers,
but in the testsuite where we manage all buffers, we flush everything.

This should drastically reduce the CI and testsuite flakiness.

Supersedes curl#7833 and curl#6064
@mback2k mback2k changed the title WIP: Testing an alternative approach to #7833 tool and tests: force flush of all buffers at end of program Mar 6, 2022
@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 8, 2022

No flakiness observed after 6 Azure CI runs. @bagder @jay

@mback2k mback2k requested review from bagder and jay March 8, 2022 19:51
@mback2k mback2k marked this pull request as ready for review March 8, 2022 19:51
@jay
Copy link
Member

jay commented Mar 10, 2022

I don't understand why this works. If the program is aborting then these flush calls shouldn't matter. Is it possible it is a coincidence that it appears to work? If a program exits normally (return from main) then the files should be flushed and closed without these additional calls.

@mback2k
Copy link
Member Author

mback2k commented Mar 10, 2022

I don't understand why this works. If the program is aborting then these flush calls shouldn't matter. Is it possible it is a coincidence that it appears to work? If a program exits normally (return from main) then the files should be flushed and closed without these additional calls.

Me neither. I think it most likely works by avoiding data getting lost in process / output forwarding chains. Remember, we are intermixing a couple of different more Unix-based technologies in the testsuite even if it is running under msys. So we have at least the native Win32 binaries, msys-based Perl and Shell, with a couple of scripts in each of those interpreters being intertwined and nested. My theory is that sometimes somewhere in those process chains, the output data get's lost in a buffer.

@mback2k
Copy link
Member Author

mback2k commented Mar 11, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mback2k
Copy link
Member Author

mback2k commented Mar 11, 2022

@jay I would like to land this for now and observe the long term statistics on Azure Pipelines after ~30 days, what do you think?

@jay
Copy link
Member

jay commented Mar 12, 2022

ok

@mback2k
Copy link
Member Author

mback2k commented Mar 20, 2022

The intermediate picture looks quite good so far, not a single test failure on Azure CI since merging this PR:
image
Source: https://dev.azure.com/daniel0244/curl/_test/analytics?definitionId=1&contextType=build filtered to master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

3 participants