Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Have MergedLinesEnumerable implement IAsyncEnumerable<string> #109
base: release-1.7
Are you sure you want to change the base?
Have MergedLinesEnumerable implement IAsyncEnumerable<string> #109
Changes from 27 commits
3fddb89
15815ce
ca60f2a
da45635
e45f1f6
87316df
7976a96
d3720b9
990605e
0ab96b0
fdd9137
750b1e0
caa9428
3b99204
6a2baae
983a28f
7104cea
f6a555a
9030874
b2b7bbe
f9f0813
8d18b9d
5151a25
97198a3
1420db9
4c68836
263d7dd
16b38df
7830e3e
ff9af41
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson
TestConsumeTwice
started failing after making this change. Explanation from ChatGPTFor posterity, I added this comment.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bartleby2718 can we try
await Task.WhenAll(task1, task2, consumeTask);
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson I've been trying to get that working, but this test fails for the async case if I do that. Not sure if the test logic is flawed (i.e. shouldn't await consumeTask if the input streams may not have been closed?) or there's a bug somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try specifically with the test being async and using
await Task.WhenAll
instead ofTask.WaitAll
? If that doesn't work, could be some kind of threading bug in 1.7There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson Yes,
MergedLinesEnumerableTestAsync
'sFuzzTest
fails butMergedLinesEnumerableTestSync
'sFuzzTest
is fine. It fails even when it's run alone, but I did notice thatTestPipeline(2)
always fails with it if all tests are run together:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson I somehow got the test to pass with
await Task.WhenAll(task1, task2, consumeTask);
! Not sure if this is the fix or it means something else needs to be fixed, but this does look promising.Let me know what you think! (FWIW the test didn't pass within 10 secodns with
if (random.Next(4) == 1)
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had to swap the order for tests to pass. Is this a red flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's revert this change and make sure it still passes. Also, does this pass or fail on main? You didn't make any changes to Pipe I think so it may be an issue with the release branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson I looked more into this and gathered some numbers, but I'm lost as to how I should debug this.
Note:
master
:PackageReference
s tonunit
3.13.3,NUnit3TestAdapter
3.17.0,Microsoft.NET.Test.Sdk
15.9.0NU1902,NU1903
toNoWarn
and setCheckEolTargetFramework
tofalse
master
(net46 / netcoreapp2.2)
release-1.7
(net462 / net6.0)
AsyncEnumerable
withTask.WaitAll(task1, task2, consumeTask)
(net462 / net6.0)
However, I noticed that a small change makes a difference.
consumeTask
, notSpinWait
.strings1
andstrings2
, the test completes within 20ms.Therefore, I believe that my
IAsyncEnumerable
implementation is flawed in a way that somehow "explodes" for bigger inputs (the threshold fluctuates, but it's somewhere between 70 and low 100s).Any idea how I should debug this? For one thing, I think replacing
Guid.NewGuid()
with a human-friendly value will help, but that's all I can think of. I can also temporarily comment outSpinWait
-related code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found that
consumeTask.Status
isWaitingForActivation
even after a few seconds (if I don'tawait
it or includeconsumeTask
in theWaitAll
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like ChatGPT came to the rescue again. (It wasn't helping a few hours ago.)
I lost its message, but it said something along the lines of "
StreamWriter
wasn't being disposed properly, causing thePipe
's InputStream to wait indefinitely."Now the test passes, but I can't run
spinWait.SpinOnce();
as often. Do you think the frequency should also be aprotected virtual
value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense; we have do dispose the writer to end the stream. Can you point me to the relevant code change?
I'm not sure I follow here. As often as what? Does it fail when it runs more often? In what way? How does the overall time for this test case compare before and after the changes (I would expect it to be the same). Who would be overriding the frequency if it were protected virtual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson
StreamWriter
changesFuzzTest
is the one where I had to change this. Note the parameter changes of the local functionWriteStrings
.spinWait.SpinOnce()
changesIn
FuzzTest
, specifically https://github.com/madelson/MedallionShell/pull/109/files#diff-68fdbc9634d30b7e1a0bb438ab37b458f0c478766fba188c44ece72f93e41cacR102-R121, I updated theif
condition fromrandom.Next(4) == 1
torandom.Next(110) == 1
, so I'm spinning left often (25% -> 0.91%). If I use a greater value forrandom.Next
(i.e. spin more often), then the test never ends (at least for like 30 minutes, after which I stop the test) only in the async case. Therefore I was wondering ifMergedLinesEnumerableTestAsync
andMergedLinesEnumerableTestSync
should use different frequencies by overriding aprotected virtual
variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson Let me know if the above makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madelson Bumping this thread!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me feel like there is a bug somewhere. It could be in the MergedLinesEnumerable changes, it could be in the test code, or it could be in the
Pipe
code.What I would suggest is to (temporarily) add some logging statements to the code like this:
My assumption is that at some point we should stop seeing log statements as the code will enter a hung state. We can then add additional logs to try to get closer and closer to the point where each thread stops.
From there, hopefully we can deduce why it is hanging.