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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 6, 2022
Merged

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

merged 1 commit into from
May 6, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented May 5, 2022

Reverts #68288

@ghost ghost assigned jkotas May 5, 2022
@ghost
Copy link

ghost commented May 5, 2022

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
Copy link
Member Author

jkotas commented May 5, 2022

Fixes #68932

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

@wfurt wfurt left a comment

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
Copy link
Member

LGTM

@jkotas
Copy link
Member Author

jkotas commented May 5, 2022

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
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs
}
return _state.StopCompletion.Task;
}

#pragma warning disable CS3016
[UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })]
Copy link
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.

Copy link
Member Author

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.

Copy link
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 jkotas force-pushed the revert-68288-mapichov/quic_interop branch from 63e6689 to a523fcc Compare May 6, 2022 02:20
@jkotas jkotas merged commit 4820674 into main May 6, 2022
@jkotas jkotas deleted the revert-68288-mapichov/quic_interop branch May 6, 2022 02:20
@jkotas
Copy link
Member Author

jkotas commented May 6, 2022

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

@rzikm rzikm mentioned this pull request May 6, 2022
Merged
@jkotas jkotas mentioned this pull request May 6, 2022
Closed
@thhous-msft
Copy link

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
Copy link
Member Author

jkotas commented May 6, 2022

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
Copy link
Member

ManickaP commented May 6, 2022

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.
@ManickaP ManickaP 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
@ghost ghost locked as resolved and limited conversation to collaborators Jun 5, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 left review comments

@wfurt wfurt wfurt approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@jkotas jkotas

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