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

API Proposal: File.ReadLinesAsync #2214

Closed
khellang opened this issue Jan 27, 2020 · 15 comments · Fixed by #66492
Closed

API Proposal: File.ReadLinesAsync #2214

khellang opened this issue Jan 27, 2020 · 15 comments · Fixed by #66492
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@khellang
Copy link
Member

khellang commented Jan 27, 2020

Now that IAsyncEnumerable<T> has landed, I'd like to propose the following new methods to the File type:

namespace System.IO {
    public static partial class File {
        public static System.Collections.Generic.IEnumerable<string> ReadLines(string path) { throw null; }
        public static System.Collections.Generic.IEnumerable<string> ReadLines(string path, System.Text.Encoding encoding) { throw null; }
+       public static System.Collections.Generic.IAsyncEnumerable<string> ReadLinesAsync(string path, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
+       public static System.Collections.Generic.IAsyncEnumerable<string> ReadLinesAsync(string path, System.Text.Encoding encoding, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
    }
}

The methods would basically be async counterparts to File.ReadLines, so they would mirror what File.ReadAllLinesAsync does, i.e. use an asynchronous, sequential StreamReader, but read line by line lazily, without allocating a list or array.

These would fill the remaining gap for async reading done by the File type, as mentioned in https://github.com/dotnet/corefx/issues/11220.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Jan 27, 2020
@MihaZupan MihaZupan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 27, 2020
@GrabYourPitchforks
Copy link
Member

Is the CancellationToken parameter at the end correct? I thought the pattern was that the IAsyncEnumerable<T>-returning component didn't take a CancellationToken and that a token was instead passed via an extension method.

@khellang
Copy link
Member Author

Is the CancellationToken parameter at the end correct? I thought the pattern was that the IAsyncEnumerable<T>-returning component didn't take a CancellationToken and that a token was instead passed via an extension method.

I think so, yes. I'm assuming the CancellationToken parameter would be annotated with an EnumeratorCancellationAttribute and used inside the actual implementation, like this:

public virtual async IAsyncEnumerable<T> ReadAllAsync([EnumeratorCancellation] CancellationToken cancellationToken = default)
{
while (await WaitToReadAsync(cancellationToken).ConfigureAwait(false))
{
while (TryRead(out T item))
{
yield return item;
}
}
}

This was the only similar API I could find in code base:

public virtual System.Collections.Generic.IAsyncEnumerable<T> ReadAllAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }

@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 28, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@khellang
Copy link
Member Author

khellang commented Apr 1, 2020

Any hint on when this will be up for review? 👼

@GrabYourPitchforks
Copy link
Member

We usually get to APIs in chronological order (by date opened) unless they're marked as blocking.

@carlossanlop carlossanlop added this to the Future milestone Jun 29, 2020
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review labels Jul 6, 2020
@carlossanlop carlossanlop modified the milestones: Future, 5.0.0 Jul 15, 2020
@terrajobst terrajobst added blocking Marks issues that we want to fast track in order to unblock other important work api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 16, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2020

Video

  • Looks good as proposed
  • The [EnumeratorCancellation] attribute might be needed in the implementation if we use yield return, but it doesn't have to be part of the ref.
namespace System.IO
{
    public static partial class File
    {
        public static IAsyncEnumerable<string> ReadLinesAsync(string path, CancellationToken cancellationToken = default);
        public static IAsyncEnumerable<string> ReadLinesAsync(string path, System.Text.Encoding encoding, CancellationToken cancellationToken = default);
    }
}

@carlossanlop
Copy link
Member

@khellang will you get a chance to work on this in the next week or two? Otherwise, I should move it to Future.

@khellang
Copy link
Member Author

@carlossanlop I've already done most of the work, but I won't be able to polish it up and submit a PR in the coming weeks as I'm on vacation 😢

@carlossanlop
Copy link
Member

No worries. Let's move it to Future then.

@carlossanlop carlossanlop modified the milestones: 5.0.0, Future Jul 29, 2020
@eiriktsarpalis
Copy link
Member

Would really love to see this included in 7.0.0

@vcsjones
Copy link
Member

@eiriktsarpalis There was a PR open for this that didn't get merged, I think this API is blocked on #20824 if I understood the PR's comments correctly.

@huoyaoyuan
Copy link
Member

@khellang Now #20824 is unblocked. Do you want to work on this again?

@khellang
Copy link
Member Author

khellang commented Mar 7, 2022

Thanks for the heads up, @huoyaoyuan! I'm pretty swamped with Other Stuff right now, so I can't commit to anything. I can try to breathe life back into #43108 when I have time, but if someone beats me to it, I won't mind 😅

@khellang khellang removed their assignment Mar 7, 2022
@danmoseley danmoseley added the help wanted [up-for-grabs] Good issue for external contributors label Mar 8, 2022
@AraHaan
Copy link
Member

AraHaan commented Mar 8, 2022

Wouldn't it be worth making ReadAllLinesAsync then call into this new API then to optimize them a bit as well?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 11, 2022
@lateapexearlyspeed
Copy link
Contributor

Hi, I would try to learn and fix it, not sure if can assign to me, here is my draft PR which will be marked as ready, thanks.

@lateapexearlyspeed
Copy link
Contributor

Hi, this PR is marked as ready for review now, thanks.

adamsitnik pushed a commit that referenced this issue Apr 29, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 29, 2022
@adamsitnik adamsitnik modified the milestones: Future, 7.0.0 Apr 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet