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

[tvOS] OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows #64930

Closed
runfoapp bot opened this issue Feb 7, 2022 · 5 comments · Fixed by #65506
Closed

[tvOS] OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows #64930

runfoapp bot opened this issue Feb 7, 2022 · 5 comments · Fixed by #65506
Labels
area-System.IO.Pipelines os-tvos Apple tvOS runtime-mono specific to the Mono runtime
Milestone
7.0.0

Comments

@runfoapp
Copy link

runfoapp bot commented Feb 7, 2022 •
edited
Loading

Runfo Tracking Issue: OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows

Build Definition Kind Run Name Console Core Dump Test Results Run Client
1619019 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1616876 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1613945 runtime-extra-platforms PR 64743 net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1612202 runtime-extra-platforms Rolling net7.0-Android-Release-arm-Mono_Release-Windows.10.Amd64.Android.Open console.log test results runclient.py
1611370 runtime-extra-platforms PR 64452 net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1602008 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1600670 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py
1596337 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log runclient.py
1595549 runtime-extra-platforms PR 64867 net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py

Build Result Summary

Day Hit Count Week Hit Count Month Hit Count
1 5 9
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO.Pipelines untriaged New issue has not been triaged by the area owner labels Feb 7, 2022
@elinor-fung elinor-fung added runtime-mono specific to the Mono runtime os-tvos Apple tvOS labels Feb 7, 2022
@ghost
Copy link

ghost commented Feb 7, 2022

Tagging subscribers to 'os-tvos': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Runfo Tracking Issue: OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows

Build Definition Kind Run Name Console Core Dump Test Results Run Client
1596337 runtime-extra-platforms Rolling net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log runclient.py
1595549 runtime-extra-platforms PR 64867 net7.0-tvOS-Release-arm64-Mono_Release-OSX.1015.Amd64.AppleTV.Open console.log test results runclient.py

Build Result Summary

Day Hit Count Week Hit Count Month Hit Count
1 2 2
Author: runfoapp[bot]
Assignees: -
Labels:

area-System.IO.Pipelines, untriaged, runtime-mono, os-tvos
Milestone: -

@elinor-fung elinor-fung changed the title OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows [tvOS] OutOfMemoryException in System.IO.Pipelines.Tests.PipeWriterTests.CompleteWithLargeWriteThrows Feb 9, 2022
@elinor-fung
Copy link
Member

3 of the last 5 rolling builds have hit this. @steveisok / @akoeplinger any ideas on who might be able to investigate what is going on here?

@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Feb 9, 2022
@steveisok steveisok added this to the 7.0.0 milestone Feb 9, 2022
@steveisok
Copy link
Member

Yes, we'll take a look at it. Seems contention related - maybe allocate a smaller amount in

runtime/src/libraries/System.IO.Pipelines/tests/PipeWriterTests.cs

Lines 233 to 258 in 99452b2

public async Task CompleteWithLargeWriteThrows()
{
var pipe = new Pipe();
pipe.Reader.Complete();
var task = Task.Run(async () =>
{
await Task.Delay(10);
pipe.Writer.Complete();
});
try
{
for (int i = 0; i < 1000; i++)
{
var buffer = new byte[10000000];
await pipe.Writer.WriteAsync(buffer);
}
}
catch (InvalidOperationException)
{
// Complete while writing
}
await task;
}
?

@elinor-fung
Copy link
Member

Looks like we're occasionally hitting OutOfMemoryException in this test on Android arm runs too: https://dev.azure.com/dnceng/public/_build/results?buildId=1612202&view=ms.vss-test-web.build-test-results-tab&runId=44895690&resultId=154816&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

@MaximLipnin
Copy link
Contributor

MaximLipnin commented Feb 17, 2022 •
edited
Loading

What if OutOfMemoryException could play the same role in tvOS case as InvalidOperationException does in the test - sorta an invalid operation ? Then we could just add another catch section. Anyway It would be weird.

akoeplinger added a commit to akoeplinger/runtime that referenced this issue Feb 17, 2022
@akoeplinger
Improve PipeWriterTests.CompleteWithLargeWriteThrows
c362498 
Allocate the buffer outside of the loop so we don't hit OOM issues.

While looking at the test I noticed that it actually had a bug too:
Nothing was asserting that we indeed throw an InvalidOperationException
when the writer is completed. Adding `Assert.ThrowsAsync` revealed that
the current loop iteration count was too low to hit the exception reliably
within the 10ms delay so instead check for the execution time.

Fixes dotnet#64930
@akoeplinger akoeplinger mentioned this issue Feb 17, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 17, 2022
akoeplinger added a commit that referenced this issue Feb 18, 2022
@akoeplinger
Improve PipeWriterTests.CompleteWithLargeWriteThrows (#65506)
3deadcf 
Allocate the buffer outside of the loop so we don't hit OOM issues.

While looking at the test I noticed that it actually had a bug too:
Nothing was asserting that we indeed throw an InvalidOperationException when the writer is completed. Adding `Assert.ThrowsAsync` revealed that the current loop iteration count was too low to hit the exception reliably
within the 10ms delay so instead check for the execution time.

Fixes #64930
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 18, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-System.IO.Pipelines os-tvos Apple tvOS runtime-mono specific to the Mono runtime
Projects
None yet
Milestone
7.0.0
Development

Successfully merging a pull request may close this issue.

Improve PipeWriterTests.CompleteWithLargeWriteThrows akoeplinger/runtime
3 participants
@steveisok @MaximLipnin @elinor-fung