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

Enable GetAsync_CancelDuringResponseBodyReceived_Buffered_TaskCanceledQuickly #32032

Closed

Conversation

alnikola
Copy link
Contributor

It enables GetAsync_CancelDuringResponseBodyReceived_Buffered_TaskCanceledQuickly test and limits its execution time by 2 minutes.
Fixes #25760

@alnikola alnikola requested a review from a team February 10, 2020 16:43
@wfurt
Copy link
Member

wfurt commented Feb 10, 2020

What would we see on failure? Should we add more instrumentation so we can nail root cause if it fails ever again?

@alnikola
Copy link
Contributor Author

alnikola commented Feb 10, 2020

@wfurt Currently, it will manifest itself as just "Operation was cancelled" since I call CancelAfter on CTS. Could you please explain what instrumentation can be used here to collect more data?

@davidsh davidsh added area-System.Net.Http test-enhancement Improvements of test source code labels Feb 10, 2020
@davidsh davidsh added this to the 5.0 milestone Feb 10, 2020
@wfurt
Copy link
Member

wfurt commented Feb 10, 2020

@wfurt Currently, it will manifest itself as just "Operation was canceled" since I call CancelAfter on CTS. Could you please explain what instrumentation can be used here to collect more data?

If we cause a core file to be created we would have pretty much everything. We could also add _output.WriteLine() to track progress or add state variable with the only purpose to track progress.

@alnikola
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola
Copy link
Contributor Author

@wfurt I'm concerned that constructing a core dump file automatically can cause a huge load on CI infra in case we accidentally break a number of tests due to some mistake. May be we need to discuss it with the test infra team and plan as a separate task.

@wfurt
Copy link
Member

wfurt commented Feb 11, 2020

@wfurt I'm concerned that constructing a core dump file automatically can cause a huge load on CI infra in case we accidentally break a number of tests due to some mistake. May be we need to discuss it with the test infra team and plan as a separate task.

My assumption is that failures are rare and hard to reproduce, right? (if not, we can debug it it) All you would need to do in this case would be assert. The rest is already ready to go.
However, that can be also done later depending on stability of the test,

@jkotas
Copy link
Member

jkotas commented Feb 11, 2020

If we cause a core file to be created we would have pretty much everything.

You can cause core file to be created by calling Environment.FailFast

constructing a core dump file automatically

We do construct core dump automatically on hard crashes, and the CI is able to handle it. It is not unusual for all tests in the core runtime PRs to crash and create core dump.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.

@alnikola alnikola added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 12, 2020
@alnikola
Copy link
Contributor Author

/azp list

@alnikola
Copy link
Contributor Author

Postponed.

@alnikola alnikola closed this Apr 15, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) test-enhancement Improvements of test source code
Projects
None yet
5 participants