Added IAsyncDisposable to TextReader by dersia · Pull Request #89477 · dotnet/runtime

dersia · 2023-07-25T22:26:19Z

added IAsyncDisposable to TextReader.
Implemnted IAsyncDisposable for StreamReader as it is implemented for StreamWriter.
Changed StreamWriter to call base.Dispose(disposing) and set _disposed to true, even if stream is not closable

I still think there are some bugs in StreamWriter and I think it makes sense to add CloseAsync() to Stream, since as of now, it is still always calling Dispose(true) which is sync, but I will open another issue for that.

… StreamReader as it is implemented for StreamWriter. Changed StreamWriter to call base.Dispose(disposing) and set _disposed to true, even if stream is not closable

ghost · 2023-07-25T22:26:31Z

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

Issue Details

added IAsyncDisposable to TextReader.
Implemnted IAsyncDisposable for StreamReader as it is implemented for StreamWriter.
Changed StreamWriter to call base.Dispose(disposing) and set _disposed to true, even if stream is not closable

fixes #88244

I still think there are some bugs in StreamWriter and I think it makes sense to add CloseAsync() to Stream, since as of now, it is still always calling Dispose(true) which is sync, but I will open another issue for that.

Author:	dersia
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

dersia · 2023-07-25T23:08:16Z

I might need some help to understand where the issue from the Analysis is coming from. Did I miss something?

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

danmoseley · 2023-07-26T00:57:10Z

The validation errors are unrelated.

jozkee

Can you please also add tests for StreamReader? You can get inspiration from https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO/tests/StreamWriter/StreamWriter.DisposeAsync.cs

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

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

jozkee · 2023-07-26T04:27:40Z

CI issues are related, you need to add the API changes to the ref assembly.

runtime/docs/coding-guidelines/updating-ref-source.md

Line 12 in d389ab9

## For System.Runtime

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

Neme12 · 2023-07-26T07:59:32Z

I still think there are some bugs in StreamWriter and I think it makes sense to add CloseAsync() to Stream, since as of now, it is still always calling Dispose(true) which is sync, but I will open another issue for that.

There's no reason to add CloseAsync. There's already DisposeAsync - that's what it should call.

dersia · 2023-07-26T18:36:05Z

Can you please also add tests for StreamReader? You can get inspiration from https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO/tests/StreamWriter/StreamWriter.DisposeAsync.cs

I wrote a test already, but I couldnt figure out how to update the ref assemblies and so I couldn't run the test, but now I know how to do it

Added StreamWriter.CloseStreamFromDisposeAsync Added same for StreamReader Added missing Refs Added Tests

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

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

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

…neccessary call to ThrowIfDisposed

dersia · 2023-07-28T07:19:04Z

all comments are addressed i think. should I resolve the conversations or is who started the conversions going to resolve it? I usually wait for the person that started it to resolve it, so I know they are OK with the change.

danmoseley · 2023-07-28T16:21:31Z

It's up to you but typically folks resolve comments when they push the change that addresses them.

jozkee · 2023-07-28T16:22:06Z

src/libraries/System.IO/tests/Stream/Stream.NullTests.cs

+            Assert.Null(await input.ReadLineAsync(default));
+            Assert.Equal("", await input.ReadToEndAsync());
+            Assert.Equal("", await input.ReadToEndAsync(default));
+            await input.DisposeAsync();


Can you please add this line to the existing test above and remove this one test to avoid duplication?

I think these to methods are testing different behavior, since Dispose is calling "Close()" on the underlying stream and DisposeAsync() is calling "DisposeAsync()" on the underlying stream. But if this isn't a concern I will happily change this.

@jozkee what do you think we should do here?

I still think you should apply my suggestion here because TestNullTextReaderAsync above is already testing Async conterparts.

jozkee · 2023-07-28T16:23:26Z

src/libraries/System.IO/tests/Stream/Stream.NullTests.cs

+            Assert.Equal(token, ex.CancellationToken);
+            ex = await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await input.ReadToEndAsync(token));
+            Assert.Equal(token, ex.CancellationToken);
+            await input.DisposeAsync();


src/libraries/System.IO/tests/Stream/Stream.NullTests.cs

src/libraries/System.IO/tests/StreamReader/StreamReader.DisposeAsync.cs

src/libraries/System.IO/tests/TextReader/TextReaderTests.cs

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

jozkee · 2023-07-28T17:30:26Z

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

+                {
+                    _disposed = true;
+                    _charLen = 0;
+                    await base.DisposeAsync().ConfigureAwait(false);


I stared at this for a long time and seems to me that this is the right thing to do, as per the docs:

An object must also call the DisposeAsync method of its base class if the base class implements IAsyncDisposable.

I don't know why we weren't doing this before. @stephentoub thoughts?

@jozkee, @stephentoub what do you think we should do here?

What you have here looks good to me.

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

jozkee · 2023-08-07T15:18:05Z

@dersia ping, I don't see your commits addressing the feedback. Bear in mind the code cut for .NET 8 RC1 is next week.

jeffhandley · 2023-08-10T18:13:25Z

I've reviewed this PR to assess whether or not we'd want to merge it in for .NET 8, and my conclusion is that we should hold off on this until .NET 9 Preview 1. If there is a compelling value proposition for this to make it into .NET 8 though, I'm happy to take more data points into consideration.

My considerations thus far are:

This involves dispose for a core type
Analyzer diagnostic CA2215 will be raised on derived types that implement IAsyncDisposable
Compiler diagnostic CS0108 will be raised on derived types that implement IAsyncDisposable
Introducing potential diagnostics for derived classes at this stage in the release has high likelihood of breaking code flow and requiring churn upstack

With the potential for the diagnostics to be introduced, getting this into .NET 9 Preview 1 will still require we scope out upstack derivatives and assess if this will disrupt code flow. If this does introduce the need to make compensating changes in other repos, then we would consider handling this as a source-incompatible breaking change. We need to assess the upstack code flow impact before merging; if compensating changes are required, then we'll want to hold off on merge until .NET 8 is locked down for GA so that we don't disrupt code flow or introduce merge conflicts.

@dersia Thank you for this contribution and the efforts you've put into it. With the plan of waiting for .NET 9 Preview 1, we would be letting this PR remain open for several weeks before merging it in.

ghost · 2023-08-25T21:01:29Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2023-09-09T00:01:15Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

added IAsyncDisposable to TextReader. Implemnted IAsyncDisposable for…

e1c8dfd

… StreamReader as it is implemented for StreamWriter. Changed StreamWriter to call base.Dispose(disposing) and set _disposed to true, even if stream is not closable

ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jul 25, 2023

danmoseley reviewed Jul 25, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs Outdated Show resolved Hide resolved

fixed comment on StreamReader.CloseStreamFromDispose

4c3b8cf

jozkee requested changes Jul 26, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 26, 2023

Neme12 reviewed Jul 26, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs Outdated Show resolved Hide resolved

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 26, 2023

Reverted Change to StreamWriter.CloseStreamFromDispose

888f957

Added StreamWriter.CloseStreamFromDisposeAsync Added same for StreamReader Added missing Refs Added Tests

build-analysis bot mentioned this pull request Jul 27, 2023

Tracking issue for CI build timeouts #76454

Closed

Neme12 reviewed Jul 27, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs Outdated Show resolved Hide resolved

Neme12 reviewed Jul 27, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs Outdated Show resolved Hide resolved

Neme12 reviewed Jul 27, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs Outdated Show resolved Hide resolved

Neme12 reviewed Jul 27, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs Outdated Show resolved Hide resolved

Fixed some Typos. Removed bool disposing from AsyncMethods. Removed u…

a4e615c

…neccessary call to ThrowIfDisposed

dersia requested a review from jozkee July 28, 2023 16:32

jozkee reviewed Jul 28, 2023

View reviewed changes

jozkee mentioned this pull request Jul 28, 2023

[Breaking Change] StreamWriter keeps working after being disposed if leaveOpen: true is used #89646

Open

jeffhandley added this to the 9.0.0 milestone Aug 10, 2023

jeffhandley mentioned this pull request Aug 10, 2023

[API Proposal]: Implement IAsyncDisposable on TextReader #88244

Open

jozkee added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 11, 2023

ghost added the no-recent-activity label Aug 25, 2023

ghost closed this Sep 9, 2023

ghost locked as resolved and limited conversation to collaborators Oct 9, 2023

This pull request was closed.

Conversation

dersia commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

dersia commented Jul 25, 2023

Uh oh!

Uh oh!

danmoseley commented Jul 26, 2023

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Neme12 commented Jul 26, 2023

Uh oh!

dersia commented Jul 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dersia commented Jul 28, 2023

Uh oh!

danmoseley commented Jul 28, 2023

Uh oh!

jozkee Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

dersia Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dersia Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

jozkee Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

jozkee Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

dersia Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

jozkee Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jozkee commented Aug 7, 2023

Uh oh!

jeffhandley commented Aug 10, 2023

Uh oh!

ghost commented Aug 25, 2023

Uh oh!

ghost commented Sep 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dersia commented Jul 25, 2023 •

edited

Loading

jozkee commented Jul 26, 2023 •

edited

Loading

dersia Jul 29, 2023 •

edited

Loading