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.

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

Remove try/catch around IsSupported property #41123

Merged

Conversation

samiizadeh
Copy link
Contributor

Description

Remove try/catch around IsSupported property

Fixes #41042

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Apr 9, 2022
@JamesNK
Copy link
Member

JamesNK commented Apr 10, 2022

@SteveSandersonMS IsSupported for QUIC has been updated to no longer throw on unsupported platforms: dotnet/runtime#67036

Is there a simple way for you to test/verify? I'd hate to remove the try/catch just to break your scenario again.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

@JamesNK Thanks for checking. Yes, it appears to work fine without the try/catch now.

This is based on me manually building stuff and overwriting assemblies in my local SDK, so it's possible I got something wrong, but it did just work so I'm reasonably confident.

@mkArtakMSFT mkArtakMSFT merged commit a8d5b6a into dotnet:main Apr 11, 2022
@ghost ghost added this to the 7.0-preview4 milestone Apr 11, 2022
@mkArtakMSFT
Copy link
Member

Thanks for your contribution, @samiizadeh!

@samiizadeh samiizadeh deleted the RemoveTryCatchAroundIsSupportedProperty branch April 11, 2022 19:30
@delneg
Copy link

delneg commented May 22, 2022

@mkArtakMSFT , @SteveSandersonMS
Trying to run Wasi SDK code, experiencing this again with both net7.0-preview4 and .net7.0-preview5 daily (7.0.100-preview.5.22267.11 to be exact) on macOS x64
Edit: Also experiencing it on Debian Linux x86_64

System.PlatformNotSupportedException: System.Net.Quic is not supported on this platform.
   at System.Net.Quic.QuicImplementationProviders.get_Default()
   at Microsoft.AspNetCore.Hosting.WebHostBuilderQuicExtensions.UseQuic(IWebHostBuilder hostBuilder)
   at Microsoft.AspNetCore.Hosting.WebHostBuilderKestrelExtensions.UseKestrel(IWebHostBuilder hostBuilder)
   at Microsoft.AspNetCore.Hosting.WebHostBuilderKestrelExtensions.UseKestrel(IWebHostBuilder hostBuilder, Action`1 options)
   at Program.main(String[] args)

@ghost
Copy link

ghost commented May 22, 2022

Hi @delneg. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@JamesNK
Copy link
Member

JamesNK commented May 22, 2022

@SteveSandersonMS Could you double check whether this is working or not?

@delneg
Copy link

delneg commented May 23, 2022

Upd: I've tried preview 3 (as mentioned here https://www.strathweb.com/2022/03/running-net-7-apps-on-wasi-on-arm64-mac/) and it works fine with 7.0.100-preview.3.22179.4 for me , but not with 7.0.100-preview.4.22252.9 or 7.0.100-preview.5.22267.11 - all of that tested on macOS x64

@ghost
Copy link

ghost commented May 23, 2022

Hi @delneg. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@rzikm
Copy link
Member

rzikm commented May 23, 2022

@delneg Can build the app as self-contained and check the System.Net.Quic.dll deployed to the publish directory with ILSpy? Even for unsupported platforms, the System.Net.Quic.QuicImplementationProviders.Default property should not throw.

@ghost
Copy link

ghost commented May 23, 2022

Hi @rzikm. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@SteveSandersonMS
Copy link
Member

[James] Could you double check whether this is working or not?

Yes, it's working for me on 7.0.100-preview.4.22252.9. Verified on Windows and Linux (Ubuntu in WSL2).

@rzikm To diagnose, I'd recommend:

  • Clean your project (remove bin and obj dirs completely)
  • Build using dotnet build /bl
  • Assuming it's still not working, upload the generated msbuild.binlog file somewhere that we can have a look

@delneg
Copy link

delneg commented May 23, 2022

FYI Just tested on macOS arm64 (M1) with preview4, also doesn't work

➜  Project git:(master) dotnet --version
7.0.100-preview.4.22252.9
➜  Project git:(master) uname -a
Darwin hostname.local 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:29:10 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T8101 arm64

@ghost
Copy link

ghost commented May 23, 2022

Hi @delneg. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@delneg
Copy link

delneg commented May 23, 2022

@delneg Can build the app as self-contained and check the System.Net.Quic.dll deployed to the publish directory with ILSpy? Even for unsupported platforms, the System.Net.Quic.QuicImplementationProviders.Default property should not throw.

@rzikm
I've tried building with dotnet publish --self-contained true
and got
-rwxr--r-- 1 user staff 361K Apr 30 00:42 System.Net.Quic.dll
in bin/Debug/net7.0/osx-arm64/publish/ directory.

I'm not sure what ILSpy should be used for (and how, never used it) though

@ghost
Copy link

ghost commented May 23, 2022

Hi @delneg. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@rzikm
Copy link
Member

rzikm commented May 23, 2022

I'm not sure what ILSpy should be used for (and how, never used it) though

It is a dissassemlber tool for .NET assemblies, but I didn't realise that you are on OSX, ILSpy is Windows only.

Regardless, based on size I believe it uses the right assembly (the one with full implementation, not PNSE throwing stubs). Can you upload the binlog as SteveSandersonMS requested?

Edit: I am unable to reproduce this with 7.0.100-preview.5.22267.11 on Linux

@ghost
Copy link

ghost commented May 23, 2022

Hi @rzikm. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@SteveSandersonMS
Copy link
Member

OK, I tracked this down. The existing 0.1.0 version of Wasi.Sdk continued to rely on the try/catch from 7.0 Preview 3, so it became broken if you updated to preview 4 where the try/catch was removed.

I've just pushed a new 0.1.1 of Wasi.Sdk that reacts to this preview 4 change and works correctly with it: https://www.nuget.org/packages/Wasi.Sdk/0.1.1

(The reason it seemed working to me even before is that I was using an up-to-date build from sources which already contained this change. Sorry for the confusion!)

@delneg
Copy link

delneg commented May 24, 2022

@SteveSandersonMS Thanks, I can confirm that the issue was indeed related to Wasi.Sdk

Now it works for me on macOS x64 with 7.0.100-preview.5.22267.11

@ghost
Copy link

ghost commented May 24, 2022

Hi @delneg. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/3: Remove try/catch around IsSupported property
8 participants