Skip to content

Conversation

@tebeco
Copy link
Contributor

@tebeco tebeco commented Aug 24, 2020

fixes #25164

Description

Found from a stackoverflow question.
System.Linq.Async has a ToAsyncEnumerable method which returns a generic class that implements IAsyncEnumerable which didn't work with our reflection logic.
https://github.com/dotnet/reactive/blob/7ad606b3dcd4bb2c6ae9622f8a59db7f8f52aa85/Ix.NET/Source/System.Linq.Async/System/Linq/Operators/ToAsyncEnumerable.cs#L34

This is a backport of #24926

Regression?

no

Risk

same as #24926
I see no obvious Risk

More:

As @BrennanConroy explained:

I mean it's possible, it's just the servicing bar is unlikely to be met. https://github.com/dotnet/aspnetcore/blob/master/docs/Servicing.md#servicing-bar

This might be rejected, but at least the PR is ready as the bug was discussed about on Gitter
This is also on LTS (since 5.0 will NOT be LTS)
eg:
The team responsible for AuthN / AuthZ Handler usually refuse to work on non-LTS, so no net5.0 for us :(

Before we were assuming if the type was generic then it must be an IAsyncEnumerable<> or some non-streaming type which is what that check was doing. But that was a wrong assumption since you can have a generic type that implements IAsyncEnumerable<> instead of being an IAsyncEnumerable directly. We then fallback to the logic below that statement that checks all interface types for IAsyncEnumerable<>. If you take a look at the added test case then you can see the structure of a type that failed the check before.

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Aug 24, 2020
@Pilchie Pilchie added the community-contribution Indicates that the PR has been added by a community member label Aug 24, 2020
@BrennanConroy
Copy link
Member

I'm not convinced we should bring this to shiproom for backporting. There is a single customer asking for it and there is a workaround provided.

@halter73
Copy link
Member

halter73 commented Sep 3, 2020

I agree this is fairly low value. But on the other hand, it's very low risk. I wouldn't change when we patch next because of this issue, but if we're going to patch anyway, I'd want to take this.

@BrennanConroy
Copy link
Member

BrennanConroy commented Sep 4, 2020

@tebeco Can you change the base branch to release/3.1 please.

@BrennanConroy BrennanConroy added this to the 3.1.x milestone Sep 4, 2020
@halter73 halter73 self-requested a review September 4, 2020 21:21
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

This looks good but it needs to target release/3.0 release/3.1.

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

Currently rebasing,
For my curiosity may I ask why 3.0 it it's now "out of support" and LTS are 2.1 / 3.1 and current it 3.1 ?

crap, my branch is based on master lmao, sorry, rebasing on release/3.1 (I suppose)
mehhhh

@halter73
Copy link
Member

halter73 commented Sep 4, 2020

Sorry, I should have said release/3.1.

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

sorry about that delay I did not check the base branch, In fact I'm surprised I managed to apply a diff on master while it was fixed in 5.0 :D
i feel stupid

@tebeco tebeco changed the base branch from master to release/3.1 September 4, 2020 22:45
@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

should be better like this
Sorry about that

@tebeco tebeco requested a review from halter73 September 4, 2020 22:48
@halter73
Copy link
Member

halter73 commented Sep 4, 2020

In fact I'm surprised I managed to apply a diff on master while it was fixed in 5.0

I'm surprised too. I wonder how that happened.

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

How bad can it go .... 😂😂😂
image

image

Look like I just selected the wrong base branch in GitHub but it was properly created
So in fact the surprise is that the CI was able to build that "diff"

@halter73
Copy link
Member

halter73 commented Sep 4, 2020

It's crazy that GitHub generated that "diff" when this was targeting master. It was going to let us merge it too, but thankfully @BrennanConroy noticed it was targeting the wrong branch.

BTW, thanks for showing me that Git Extensions has a dark mode 😄 I really like it. Do you know if there's a way to make the title bar match? I know I could change it to globally show a dark "accent color" for every title bar, but that messes with other apps.

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

It's crazy that GitHub generated that "diff" when this was targeting master

oO right ... something else went wrong and it's a bit hard to find out what was it ...

kudos to @BrennanConroy

Do you know if there's a way to make the title bar match

Nop, I knew something was off, but now I really see it WHYYYYYYYY

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

(restoring branch / running it to see if test fails locally too)
can't tell from build log if related or not

can you confirm if these E2E runs on build.cmd or is there like build.cmd -e2e or so to be sure ?

@BrennanConroy BrennanConroy added the Servicing-consider Shiproom approval is required for the issue label Sep 4, 2020
@ghost
Copy link

ghost commented Sep 4, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@BrennanConroy
Copy link
Member

The failures are due to some SameSite cookie issues due to Chrome updating. This PR wont pass until we merge some changes into 3.1 first. We'll re-run this when that happens.

@tebeco
Copy link
Contributor Author

tebeco commented Sep 4, 2020

do I need to change the format of the template and remove all the "MORE" from the PR description for the "servicing model" ?

@tebeco
Copy link
Contributor Author

tebeco commented Sep 6, 2020

unrelated
@halter73 you can check gitextensions PR 8435 (avoiding linking unrelated PR)

@wtgodbe
Copy link
Member

wtgodbe commented Sep 8, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wtgodbe
Copy link
Member

wtgodbe commented Sep 8, 2020

The 3 red jobs are from friday - weird that github is still showing them here. CI is actually green

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 10, 2020
@leecow leecow modified the milestones: 3.1.x, 3.1.9 Sep 10, 2020
@wtgodbe wtgodbe merged commit c12149e into dotnet:release/3.1 Sep 11, 2020
@tebeco tebeco deleted the backport-iae-fix-pr-24926 branch February 9, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-signalr Includes: SignalR clients and servers community-contribution Indicates that the PR has been added by a community member Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[3.1.x - SignalR] Duplex Stream using IAsyncEnumerable fail to Bind argument while it works with ChannelReader, or on 5.x with both

6 participants