Skip to content

Revert "[QUIC] Adopted msquic generated interop" - #68940

Merged
jkotas merged 1 commit into
mainfrom
revert-68288-mapichov/quic_interop
May 6, 2022
Merged

Revert "[QUIC] Adopted msquic generated interop"#68940
jkotas merged 1 commit into
mainfrom
revert-68288-mapichov/quic_interop

Conversation

@jkotas

@jkotasjkotas commented May 5, 2022

Copy link
Copy Markdown
Member

Reverts #68288

@ghostghost assigned jkotasMay 5, 2022
@ghost

ghost commented May 5, 2022

Copy link
Copy Markdown

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

Issue Details

Reverts #68288

Author:jkotas
Assignees:jkotas
Labels:

area-System.Net.Quic

Milestone:-

@jkotas

jkotas commented May 5, 2022

Copy link
Copy Markdown
MemberAuthor

Fixes #68932

@jkotas
jkotas requested a review from marek-safar as a code ownerMay 5, 2022 22:36
wfurt
wfurt approved these changes May 5, 2022
View reviewed changes

@wfurtwfurt 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.

LGTM. Why this did not show up on the original PR @jkotas ?

@SamMonoRT

SamMonoRT commented May 5, 2022

Copy link
Copy Markdown
Member

LGTM

@jkotas

jkotas commented May 5, 2022

Copy link
Copy Markdown
MemberAuthor

We do not run all CI legs on all PRs. We only run subset of legs that we expect to be impacted by the change, based on the set of files that the change is modifying.

It works great as long as there no bugs. Something in the Quic change exposed existing crashing bug in Mono and all PRs where we decide to run Mono legs are failing now.

am11
am11 reviewed May 5, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs
}

#pragma warning disable CS3016
[UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })]

@am11am11May 5, 2022

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.

Assuming the assertion was:

condition `out_obj' not met

according to #68851 (comment), then it is tracked by #57361 and #52977. Perhaps the fix is to use non-array argument in the attribute:

- [UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })]+ [UnmanagedFunctionPointer(CallingConvention.Cdecl)]

here and in MsQuicStream.cs.

@jkotasjkotasMay 5, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It would also require reverting back to using delegates in number of places, partially defeating the point of this change.

I think we either need to get the Mono bug fixed soon; or disable Mono AOT for the Quic library.

@am11am11May 5, 2022

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.

Agreed. cc @lambdageek (another example where #57361 and #52977 are blocking array argument usage)

@jkotas
jkotasforce-pushed the revert-68288-mapichov/quic_interop branch from 63e6689 to a523fccCompareMay 6, 2022 02:20
@jkotas
jkotas merged commit 4820674 into mainMay 6, 2022
@jkotas
jkotas deleted the revert-68288-mapichov/quic_interop branch May 6, 2022 02:20
@jkotas

jkotas commented May 6, 2022

Copy link
Copy Markdown
MemberAuthor

@ManickaP I am sorry to revert your PR to get CI back on track. Could you please work with the Mono team on finding the best way to address the failure? I see two options. I think we either need to get the Mono AOT bug fixed or we need to find a way to workaround it (e.g. by disabling Mono AOT for System.Net.Quic).

@rzikmrzikm mentioned this pull request May 6, 2022
Merged
@jkotasjkotas mentioned this pull request May 6, 2022
Closed
@thhous-msft

thhous-msft commented May 6, 2022

Copy link
Copy Markdown

Just to double check, this isn't something we'd need to fix on the MsQuic generated interop layer side, right? Its a mono specific JIT bug?

@jkotas

jkotas commented May 6, 2022

Copy link
Copy Markdown
MemberAuthor

Just to double check, this isn't something we'd need to fix on the MsQuic generated interop layer side, right? Its a mono specific JIT bug?

Correct.

@ManickaP

ManickaP commented May 6, 2022

Copy link
Copy Markdown
Member

This is a bummer 😢 I'll disable mono AOT tests for S.N.Quic and try to put up another PR.

ManickaP added a commit to ManickaP/runtime that referenced this pull request May 7, 2022
@ManickaP
Revert "Revert "[QUIC] Adopted msquic generated interop (dotnet#68288)…
b8f3819
…" (dotnet#68940)"
This reverts commit 4820674.
@ManickaPManickaP mentioned this pull request May 9, 2022
Closed
ManickaP added a commit that referenced this pull request May 11, 2022
@ManickaP
"[QUIC] Adopted msquic generated interop (#68288)" attempt no 2 (#69009)
122c9d0
* Revert "Revert "[QUIC] Adopted msquic generated interop (#68288)" (#68940)"
This reverts commit 4820674.
* Exclude S.N.Quic from Mono AOT
* Fix#68954
@ghostghost locked as resolved and limited conversation to collaborators Jun 5, 2022
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
+1 more reviewer
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jkotasjkotas

Labels

area-System.Net.Quic

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@jkotas@SamMonoRT@thhous-msft@ManickaP@am11@wfurt@karelz