Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Restore Channels ReadAsync implementation - #26934

Merged
tarekgh merged 4 commits into
dotnet:masterfrom
tarekgh:Restore-ReadAsync-implementation-for-Channels
Feb 9, 2018
Merged

Restore Channels ReadAsync implementation#26934
tarekgh merged 4 commits into
dotnet:masterfrom
tarekgh:Restore-ReadAsync-implementation-for-Channels

Conversation

@tarekgh

@tarekgh tarekgh commented Feb 7, 2018

Copy link
Copy Markdown
Member

No description provided.

@tarekgh

tarekgh commented Feb 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@stephentoub I have brought back the implementation of ReadSync on all Channels classes. one question, do we really need to use AutoResetAwaiter? I have ported it because it was used in the SingleConsumerUnboundedChannel but I am not sure if it is really needed.

@tarekgh
tarekgh requested a review from stephentoub February 7, 2018 21:09
@tarekgh

tarekgh commented Feb 7, 2018

Copy link
Copy Markdown
Member Author

cc @joshfree

@stephentoub

stephentoub commented Feb 8, 2018

Copy link
Copy Markdown
Member

do we really need to use AutoResetAwaiter?

Nope. It was there for GetAwaiter, which was removed.

@tarekgh

tarekgh commented Feb 8, 2018

Copy link
Copy Markdown
Member Author

good, I'll get rid of AutoResetAwaiter then.

stephentoub
stephentoub reviewed Feb 8, 2018
View reviewed changes

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing all this back, @tarekgh. Were there deleted tests as well, or did I already bring those back when I added back the base virtual method? It'd be good to do a code coverage run locally to see what's not covered; previously this should have all had close to 100% coverage, so I expect any missing lines/branches (other than ones that can only be hit in race conditions) would indicate some tests still in history.

@stephentoub

stephentoub commented Feb 8, 2018

Copy link
Copy Markdown
Member

@dotnet-bot test this please

@tarekgh

tarekgh commented Feb 8, 2018

Copy link
Copy Markdown
Member Author

@stephentoub The code coverage is almost 100% in all areas except

System.Threading.Channels.ChannelReader`1    32.1%    14.2%

And that is expected as this code is not consumed by any of our concrete channels. I'll try to write a test force exercising this code too so we'll go back to close to 100%
I don't see any old/removed tests can help in this scenario.

@stephentoub

stephentoub commented Feb 8, 2018

Copy link
Copy Markdown
Member

Ok. Thanks, @tarekgh.

stephentoub
stephentoub reviewed Feb 8, 2018
View reviewed changes
Comment thread src/System.Threading.Channels/tests/ChannelTests.cs Outdated
var reader = channel.Reader;
var writer = channel.Writer;

// 1- do it through synchoronouse TryRead()

@stephentoub stephentoub Feb 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spelling of synchronous

stephentoub
stephentoub reviewed Feb 8, 2018
View reviewed changes
Comment thread src/System.Threading.Channels/tests/ChannelTests.cs Outdated
{
WrapperChannel<int> channel = new WrapperChannel<int>(10);
var reader = channel.Reader;
var writer = channel.Writer;

@stephentoub stephentoub Feb 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you use the actual type names instead of var here and below where it's not stated on the right-hand side?

@tarekgh

tarekgh commented Feb 9, 2018

Copy link
Copy Markdown
Member Author

@MattGal is this known issue?

https://ci3.dot.net/job/dotnet_corefx/job/master/job/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/8265/

  System.IO.FileSystem.Watcher.Tests -> /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/bin/OSX.AnyCPU.Debug/System.IO.FileSystem.Watcher.Tests/netstandard/System.IO.FileSystem.Watcher.Tests.dll
  System.Net.HttpListener.Tests -> /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/bin/OSX.AnyCPU.Debug/System.Net.HttpListener.Tests/netstandard/System.Net.HttpListener.Tests.dll
    File "/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/dumpling.py", line 1
      <!DOCTYPE html>
      ^
  SyntaxError: invalid syntax
  /usr/bin/python: can't open file '/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/tempHome/.dumpling/dumpling.py': [Errno 2] No such file or directory
  
  
  /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/dir.traversal.targets(81,3): warning MSB4011: "/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/Dumpling.targets" cannot be imported again. It was already imported at "/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/tests.targets (558,3)". This is most likely a build authoring error. This subsequent import will be ignored. [/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/tests.builds]

I am merging this change and we can log an issue if this is unknown

@tarekgh
tarekgh merged commit ce0de73 into dotnet:master Feb 9, 2018
@MattGal

MattGal commented Feb 9, 2018

Copy link
Copy Markdown
Member

@tarekgh this was addressed by dotnet/buildtools#1900 , and is a symptom of the dumpling server being offline.

A-And pushed a commit to A-And/corefx that referenced this pull request Feb 21, 2018
@tarekgh @A-And
Restore Channels ReadAsync implementation (dotnet#26934)
ab91d90 
* Restore Channels ReadAsync implementation

* Remove AutoResetAwaiter

* Add Base class  ReadAsync test

* address the feedback
@karelz karelz added this to the 2.1.0 milestone Mar 10, 2018
@tarekgh
tarekgh deleted the Restore-ReadAsync-implementation-for-Channels branch May 25, 2018 17:24
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@tarekgh
Restore Channels ReadAsync implementation (dotnet/corefx#26934)
319a402 
* Restore Channels ReadAsync implementation

* Remove AutoResetAwaiter

* Add Base class  ReadAsync test

* address the feedback


Commit migrated from dotnet/corefx@ce0de73
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@stephentoub stephentoub stephentoub approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekgh tarekgh

Labels

area-System.Threading.Channels

Projects

None yet

Milestone

2.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tarekgh @stephentoub @MattGal @karelz