issue-2214 Create File.ReadLinesAsync() #66492

lateapexearlyspeed · 2022-03-11T06:46:54Z

Fix #2214 , will mark it as ready when it is ready, thanks.

But there are some notes, please correct my understand on its behavior in advance:
"File not found" related exceptions will be thrown at beginning of ReadLinesAsync() which is same as ReadLines(), but followings are different with ReadLines() if I assume they are different method so not consider compatible with 4.0:

IEnumerator<T> instances from the same IEnumerable<T> will not share same underlying reader
Not dispose just after read to end of stream reader.

dotnet-issue-labeler · 2022-03-11T06:46:58Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-03-11T06:47:01Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #2214 , will mark it as ready when it is ready, thanks.

But there are some notes, please correct my understand on its behavior in advance:
"File not found" related exceptions will be thrown at beginning of ReadLinesAsync() which is same as ReadLines(), but followings are different with ReadLines() if I assume they are different method so not consider compatible with 4.0:

IEnumerator<T> instances from the same IEnumerable<T> will not share same underlying reader
Not dispose just after read to end of stream reader.

Author:	lateapexearlyspeed
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`
Milestone:	-

AraHaan · 2022-03-11T07:22:24Z

I feel like ReadAllLines should be optimized to use ReadLines() and then call ToArray() on that, same with the async version of it.

stephentoub · 2022-03-12T20:52:09Z

I feel like ReadAllLines should be optimized to use ReadLines() and then call ToArray() on that, same with the async version of it.

Why do you believe that's an improvement?

AraHaan · 2022-03-12T21:00:37Z

I feel like ReadAllLines should be optimized to use ReadLines() and then call ToArray() on that, same with the async version of it.

Why do you believe that's an improvement?

Because it looks like it duplicates the logic from ReadLines() in part of the ReadAllLines() function.

stephentoub · 2022-03-13T02:31:40Z

I feel like ReadAllLines should be optimized to use ReadLines() and then call ToArray() on that

Why do you believe that's an improvement?

Because it looks like it duplicates the logic

There's a maintainability argument, but it's not an optimization. ReadLines() strictly has more overhead.

stephentoub · 2022-03-14T01:27:25Z

src/libraries/System.Private.CoreLib/src/System/IO/AsyncReadLinesEnumerable.cs

+        }
+    }
+
+    internal sealed class AsyncReadLinesEnumerator : IAsyncEnumerator<string>


We shouldn't implement this manually; as written the implementation is missing out on some important optimizations. We should instead use the async iterator support provided by the compiler.

Fixed, and this iterator has following behaviors, please check if it is our expected ones (they should be same behaviors of ReadLines though):

Check and throw file related exception if occur upfront before GetEnumerator
So that leaking first stream reader if not even foreach
Share one stream reader for multiple enumerator until first round iteration finishes.

Hi @stephentoub This PR (with async iterator by compiler as suggested) is ready for review now, please review, thanks.

adamsitnik

@lateapexearlyspeed thank you for another great contribution! PTAL at my comments, I would like to get a better understanding of why the StreamReader can be disposed an re-created more than once

src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllLinesAsync.cs

adamsitnik · 2022-04-26T14:51:55Z

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

+
+        private static async IAsyncEnumerable<string> IterateFileLinesAsync(DisposingFlagStreamReader sr, string path, Encoding encoding, CancellationToken ctEnumerable, [EnumeratorCancellation] CancellationToken ctEnumerator = default)
+        {
+            if (sr.IsDisposed)


in what scenario is it possible for the DisposingFlagStreamReader to be disposed here?

PTAL at my comments, I would like to get a better understanding of why the StreamReader can be disposed an re-created more than once

Answer together here :)

in what scenario is it possible for the DisposingFlagStreamReader to be disposed here?

Before start this code, I checked existing IEnumerable File.ReadLines() related code and related comment and got some background that we would move first streamReader allocation before involve Enumerator (so pass streamReader to this IterateFileLinesAsync()) so that exceptions such as DirectoryNotFoundException and FileNotFoundException are thrown directly by File.ReadLinesAsync (which the user probably expects), rather than throw until call Enumerator.MoveNext.
And there was comment to suggest using compiler-generated Iterator to implement.

Because generated enumerable stores first time passed streamReader reference, so here we check if current streamReader reference has been disposed already for scenario: when previous user has consumed all lines and then call GetEnumerator against same enumerable).

IAsyncEnumerable<string> enumerable = File.ReadLinesAsync("path"); await foreach (string line in enumerable) { // consume line; } // streamReader is disposed now // can use same enumerable to create another enumerator and work properly await foreach (string line in enumerable) { // consume line; }

@lateapexearlyspeed thanks for the explanation! Now I can see that we do something similar for File.ReadLines:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/ReadLinesIterator.cs

Lines 64 to 70 in a077f27

// NOTE: To maintain the same behavior with the previous yield-based

// iterator in 4.0, we have all the IEnumerator<T> instances share the same

// underlying reader. If we have already been disposed, _reader will be null,

// which will cause CreateIterator to simply new up a new instance to start up

// a new iteration. We cannot change this behavior due to compatibility

// concerns.

return CreateIterator(_path, _encoding, _reader);

I wonder whether we need a new type for DisposingFlagStreamReader. The ReadLinesAsync method could create a FileStream and pass it here and then we could use FileStream.CanRead to figure out whether it has been disposed or not:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs

Line 73 in a077f27

public sealed override bool CanRead => !_fileHandle.IsClosed && (_access & FileAccess.Read) != 0;

@stephentoub just to be extra sure I assume that we want to have this behavior?

Fixed by using FileStream.CanRead. Use StreamReader.BaseStream to get inner fileStream so can pass and reuse StreamReader method to read lines.

AraHaan · 2022-04-27T06:59:41Z

too bad we cant avoid the disposable and read it's contents line for line directly into a span and return it as an array (using async with linq).

AraHaan · 2022-04-27T07:02:53Z

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

+                sr = new DisposingFlagStreamReader(path, encoding);
+            }
+
+            using (sr)


shouldn't this be async?

Also note: this could be changed from block using form to normal using form from here (after it leaves scope it will be disposed).

AraHaan · 2022-04-28T10:50:52Z

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

@@ -343,7 +343,7 @@ public static IAsyncEnumerable<string> ReadLinesAsync(string path, Encoding enco
        {
            Validate(path, encoding);

-            var sr = new DisposingFlagStreamReader(path, encoding); // Move first streamReader allocation here so to throw related file exception upfront, which will cause known leaking if user never actually foreach's over the enumerable
+            StreamReader sr = AsyncStreamReader(path, encoding); // Move first streamReader allocation here so to throw related file exception upfront, which will cause known leaking if user never actually foreach's over the enumerable


What if they used linq query/method style with the function instead of foreach (and they use .ToList() at the end of it to execute the linq query/methods)? Will it leak in that case?

adamsitnik

LGTM, thank you @lateapexearlyspeed !

issue-2214 Initial commit: create an AsyncReadLinesEnumerable.

4502a70

dotnet-issue-labeler bot added area-System.IO new-api-needs-documentation labels Mar 11, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 11, 2022

lateapexearlyspeed changed the title ~~issue-2214 Initial commit: create an AsyncReadLinesEnumerable.~~ issue-2214 Create File.ReadLinesAsync() Mar 11, 2022

lateapexearlyspeed mentioned this pull request Mar 11, 2022

API Proposal: File.ReadLinesAsync #2214

Closed

stephentoub reviewed Mar 14, 2022

View reviewed changes

lateapexearlyspeed added 4 commits March 14, 2022 15:06

Fix comment: implement by async yield iterator.

cf2b599

Add ReadLinesAsync(path, ct); add xml doc.

84c37c5

return val doc.

18b074d

Add tests.

51d1c24

lateapexearlyspeed marked this pull request as ready for review March 22, 2022 09:19

adamsitnik reviewed Apr 26, 2022

View reviewed changes

adamsitnik self-assigned this Apr 26, 2022

lateapexearlyspeed added 2 commits April 27, 2022 12:23

Fix comment: refine test code to reduce duplicated cases.

c7f4566

Add 'sealed'.

a092c73

AraHaan reviewed Apr 27, 2022

View reviewed changes

Fix comment: remove DisposingFlagStreamReader.

640f1b2

AraHaan reviewed Apr 28, 2022

View reviewed changes

adamsitnik approved these changes Apr 29, 2022

View reviewed changes

adamsitnik merged commit 8b6f09a into dotnet:main Apr 29, 2022

adamsitnik added this to the 7.0.0 milestone Apr 29, 2022

ghost locked as resolved and limited conversation to collaborators May 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue-2214 Create File.ReadLinesAsync() #66492

issue-2214 Create File.ReadLinesAsync() #66492

lateapexearlyspeed commented Mar 11, 2022

dotnet-issue-labeler bot commented Mar 11, 2022

ghost commented Mar 11, 2022

AraHaan commented Mar 11, 2022

stephentoub commented Mar 12, 2022

AraHaan commented Mar 12, 2022

stephentoub commented Mar 13, 2022

stephentoub Mar 14, 2022

lateapexearlyspeed Mar 14, 2022

lateapexearlyspeed Mar 22, 2022

adamsitnik left a comment

adamsitnik Apr 26, 2022

lateapexearlyspeed Apr 27, 2022

adamsitnik Apr 27, 2022

adamsitnik Apr 27, 2022

lateapexearlyspeed Apr 28, 2022

AraHaan commented Apr 27, 2022 •

edited

Loading

AraHaan Apr 27, 2022 •

edited

Loading

AraHaan Apr 28, 2022 •

edited

Loading

adamsitnik left a comment

	// NOTE: To maintain the same behavior with the previous yield-based
	// iterator in 4.0, we have all the IEnumerator<T> instances share the same
	// underlying reader. If we have already been disposed, _reader will be null,
	// which will cause CreateIterator to simply new up a new instance to start up
	// a new iteration. We cannot change this behavior due to compatibility
	// concerns.
	return CreateIterator(_path, _encoding, _reader);

issue-2214 Create File.ReadLinesAsync() #66492

issue-2214 Create File.ReadLinesAsync() #66492

Conversation

lateapexearlyspeed commented Mar 11, 2022

dotnet-issue-labeler bot commented Mar 11, 2022

ghost commented Mar 11, 2022

AraHaan commented Mar 11, 2022

stephentoub commented Mar 12, 2022

AraHaan commented Mar 12, 2022

stephentoub commented Mar 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AraHaan commented Apr 27, 2022 • edited Loading

AraHaan Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

AraHaan Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

AraHaan commented Apr 27, 2022 •

edited

Loading

AraHaan Apr 27, 2022 •

edited

Loading

AraHaan Apr 28, 2022 •

edited

Loading