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

Android HttpClientHandler throws wrong exception type #56089

Closed
jonathanpeppers opened this issue Jul 21, 2021 · 2 comments · Fixed by #56334
Closed

Android HttpClientHandler throws wrong exception type #56089

jonathanpeppers opened this issue Jul 21, 2021 · 2 comments · Fixed by #56334
Assignees
@steveisok
Labels
area-System.Net.Http os-android os-ios Apple iOS
Milestone
6.0.0

Comments

@jonathanpeppers
Copy link
Member

Description

Code such as this should throw a ArgumentOutOfRangeException:

new HttpClientHandler().MaxAutomaticRedirections = 0;

We have some tests checking this, and they throw TargetInvocationException instead:

07-21 08:08:26.036 30461 30478 I NUnit   : System.Reflection.TargetInvocationException : Arg_TargetInvocationException
07-21 08:08:26.036 30461 30478 I NUnit   :   ----> System.ArgumentOutOfRangeException : The specified value must be greater than 0 Arg_ParamName_Name, value
07-21 08:08:26.036 30461 30478 I NUnit   : ArgumentOutOfRange_ActualValue, 0
07-21 08:08:26.036 30461 30478 I NUnit   : 
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in System.Private.CoreLib.dll:token 0x6002810+0x89
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters) in System.Private.CoreLib.dll:token 0x6002779+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String , Object[] ) in System.Net.Http.dll:token 0x60001cd+0x2d
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.SetMaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001dc+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.set_MaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001da+0x7
07-21 08:08:26.036 30461 30478 I NUnit   :    at Xamarin.Android.NetTests.HttpClientHandlerTestBase.Properties_Invalid() in Mono.Android.NET-Tests.dll:token 0x600001c+0x7
...

We might need exception handling here to throw the inner exception if it hits a TargetInvocationException:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs

Lines 417 to 428 in 0733681

private object InvokeNativeHandlerMethod(string name, params object?[] parameters)
{
MethodInfo? method;
if (!s_cachedMethods.TryGetValue(name, out method))
{
method = _nativeHandler!.GetType()!.GetMethod(name);
s_cachedMethods[name] = method;
}
return method!.Invoke(_nativeHandler, parameters)!;
}

Configuration

dotnet --version
6.0.100-rc.1.21369.3

I believe this also happens in Preview 7.

Regression?

I believe this was working in Preview 6, because the Android http handler implementation was not complete yet.

/cc @steveisok
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Jul 21, 2021
@ghost
Copy link

ghost commented Jul 21, 2021

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

Issue Details

Description

Code such as this should throw a ArgumentOutOfRangeException:

new HttpClientHandler().MaxAutomaticRedirections = 0;

We have some tests checking this, and they throw TargetInvocationException instead:

07-21 08:08:26.036 30461 30478 I NUnit   : System.Reflection.TargetInvocationException : Arg_TargetInvocationException
07-21 08:08:26.036 30461 30478 I NUnit   :   ----> System.ArgumentOutOfRangeException : The specified value must be greater than 0 Arg_ParamName_Name, value
07-21 08:08:26.036 30461 30478 I NUnit   : ArgumentOutOfRange_ActualValue, 0
07-21 08:08:26.036 30461 30478 I NUnit   : 
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in System.Private.CoreLib.dll:token 0x6002810+0x89
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters) in System.Private.CoreLib.dll:token 0x6002779+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String , Object[] ) in System.Net.Http.dll:token 0x60001cd+0x2d
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.SetMaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001dc+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.set_MaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001da+0x7
07-21 08:08:26.036 30461 30478 I NUnit   :    at Xamarin.Android.NetTests.HttpClientHandlerTestBase.Properties_Invalid() in Mono.Android.NET-Tests.dll:token 0x600001c+0x7
...

We might need exception handling here to throw the inner exception if it hits a TargetInvocationException:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs

Lines 417 to 428 in 0733681

private object InvokeNativeHandlerMethod(string name, params object?[] parameters)
{
MethodInfo? method;
if (!s_cachedMethods.TryGetValue(name, out method))
{
method = _nativeHandler!.GetType()!.GetMethod(name);
s_cachedMethods[name] = method;
}
return method!.Invoke(_nativeHandler, parameters)!;
}

Configuration

dotnet --version
6.0.100-rc.1.21369.3

I believe this also happens in Preview 7.

Regression?

I believe this was working in Preview 6, because the Android http handler implementation was not complete yet.

/cc @steveisok

Author: jonathanpeppers
Assignees: -
Labels:

area-System.Net.Http, untriaged
Milestone: -

@ghost
Copy link

ghost commented Jul 21, 2021

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Code such as this should throw a ArgumentOutOfRangeException:

new HttpClientHandler().MaxAutomaticRedirections = 0;

We have some tests checking this, and they throw TargetInvocationException instead:

07-21 08:08:26.036 30461 30478 I NUnit   : System.Reflection.TargetInvocationException : Arg_TargetInvocationException
07-21 08:08:26.036 30461 30478 I NUnit   :   ----> System.ArgumentOutOfRangeException : The specified value must be greater than 0 Arg_ParamName_Name, value
07-21 08:08:26.036 30461 30478 I NUnit   : ArgumentOutOfRange_ActualValue, 0
07-21 08:08:26.036 30461 30478 I NUnit   : 
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in System.Private.CoreLib.dll:token 0x6002810+0x89
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters) in System.Private.CoreLib.dll:token 0x6002779+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String , Object[] ) in System.Net.Http.dll:token 0x60001cd+0x2d
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.SetMaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001dc+0x0
07-21 08:08:26.036 30461 30478 I NUnit   :    at System.Net.Http.HttpClientHandler.set_MaxAutomaticRedirections(Int32 ) in System.Net.Http.dll:token 0x60001da+0x7
07-21 08:08:26.036 30461 30478 I NUnit   :    at Xamarin.Android.NetTests.HttpClientHandlerTestBase.Properties_Invalid() in Mono.Android.NET-Tests.dll:token 0x600001c+0x7
...

We might need exception handling here to throw the inner exception if it hits a TargetInvocationException:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs

Lines 417 to 428 in 0733681

private object InvokeNativeHandlerMethod(string name, params object?[] parameters)
{
MethodInfo? method;
if (!s_cachedMethods.TryGetValue(name, out method))
{
method = _nativeHandler!.GetType()!.GetMethod(name);
s_cachedMethods[name] = method;
}
return method!.Invoke(_nativeHandler, parameters)!;
}

Configuration

dotnet --version
6.0.100-rc.1.21369.3

I believe this also happens in Preview 7.

Regression?

I believe this was working in Preview 6, because the Android http handler implementation was not complete yet.

/cc @steveisok

Author: jonathanpeppers
Assignees: -
Labels:

area-System.Net.Http, os-android, os-ios, untriaged
Milestone: -

@steveisok steveisok added this to the 6.0.0 milestone Jul 21, 2021
@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2021
@steveisok steveisok self-assigned this Jul 21, 2021
jonathanpeppers added a commit to dotnet/android that referenced this issue Jul 21, 2021
@jonathanpeppers
[tests] changes for AndroidClientHandlerTests to pass
04b67b4 
* MaxAutomaticRedirections should throw if <= 0
* UseCookies defaults to true
* CookieContainer creates a new instance on first call
* Catch TargetInvocationException, see: dotnet/runtime#56089
* Catch PlatformNotSupportedException for ClientCertificateOptions
jonathanpeppers added a commit to dotnet/android that referenced this issue Jul 21, 2021
@jonathanpeppers
[tests] changes for AndroidClientHandlerTests to pass
c146362 
* MaxAutomaticRedirections should throw if <= 0
* UseCookies defaults to true
* CookieContainer creates a new instance on first call
* Catch TargetInvocationException, see: dotnet/runtime#56089
* Catch PlatformNotSupportedException for ClientCertificateOptions
steveisok pushed a commit to steveisok/runtime that referenced this issue Jul 26, 2021
Have HttpClientHandler throw the InnerException when catching TargetI…
c5b41e9 
…nvocationException

Originally on mobile workloads when UseNativeHttpHandler is set to true, all reflection method invokes
bubbled up a TargetInvocationException.  To make the details a bit more readable, we will instead rethrow
the InnerException.

Fixes dotnet#56089
@steveisok steveisok mentioned this issue Jul 26, 2021
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 26, 2021
steveisok added a commit that referenced this issue Jul 27, 2021
@steveisok
Have HttpClientHandler throw the InnerException when catching TargetI…
baf0cd8 
…nvocationException (#56334)

Originally on mobile workloads when UseNativeHttpHandler is set to true, all reflection method invokes
bubbled up a TargetInvocationException.  To make the details a bit more readable, we will instead rethrow
the InnerException.

Fixes #56089
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 27, 2021
jonpryor pushed a commit to dotnet/android that referenced this issue Aug 21, 2021
@dotnet-maestro @jonathanpeppers
Bump to dotnet/installer@7cfb26e4 6.0.100-rc.1.21420.5 (#6205)
b2d9659 
Changes: dotnet/installer@78a1bc3...7cfb26e
Changes: dotnet/linker@5b2391c...5851f6d
Changes: dotnet/runtime@14b34eb...fe6bd2d

Context: https://github.com/dotnet/runtime/blob/0f5b75344d5858d76da403735ee34c71d9d69d54/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L776
Context: dotnet/runtime@57dd919
Context: dotnet/runtime#56089
Context: dotnet/runtime#57800

Updates:

  * Microsoft.Dotnet.Sdk.Internal: from 6.0.100-rc.1.21418.8 to 6.0.100-rc.1.21420.5
  * Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.6.21416.1 to 6.0.100-preview.6.21419.1
  * Microsoft.NETCore.App.Ref: from 6.0.0-rc.1.21417.1 to 6.0.0-rc.1.21419.17

Update `Xamarin.Android.Net.AndroidMessageHandler` so that additional
properties are specified.  Commit dotnet/runtime@57dd919a updated
`HttpMessageHandler` so that additional properties were no longer
"unsupported" on Android, meaning they needed to exist.  As they
didn't yet exist, the linker emitted warnings:

	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.GetClientCertificateOptions(): No members were resolved for 'get_ClientCertificateOptions'.
	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.GetClientCertificates(): No members were resolved for 'get_ClientCertificates'.
	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.SetClientCertificateOptions(ClientCertificateOption): No members were resolved for 'set_ClientCertificateOptions'.

We need to add the missing members that are now expected:

	partial class AndroidMessageHandler {
	    public ClientCertificateOption ClientCertificateOptions { get; set; }
	    public X509CertificateCollection ClientCertificates { get; set; }
	    // …
	}

Note that these new properties *must* be public, as
`Type.GetMethod(string)` is used, which only finds public members.

We can also update `AndroidClientHandlerTests.cs` and remove the
`catch(TargetInvocationException)` blocks added in 1e5bfa3, as 
dotnet/runtime#56089 has been fixed.

Begin ignoring `AotTests.NoSymbolsArgShouldReduceAppSize()`, as it
started failing.  See dotnet/runtime#57800.

Finally, `SslProtocols.Tls13` is only valid on API-29+.
Update `AndroidMessageHandler.SslProtocols` so that
`SslProtocols.Tls13` is only used on appropriate platforms.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
jonathanpeppers added a commit to dotnet/android that referenced this issue Aug 22, 2021
@dotnet-maestro @jonathanpeppers
Bump to dotnet/installer@5c2d8ef 6.0.100-rc.2.21418.44 (#6204)
1dbb547 
Changes: dotnet/installer@78a1bc3...5c2d8ef
Changes: dotnet/linker@5b2391c...5b2391c
Changes: dotnet/runtime@14b34eb...5a5d7f0

Context: https://github.com/dotnet/runtime/blob/0f5b75344d5858d76da403735ee34c71d9d69d54/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L776
Context: dotnet/runtime@57dd919
Context: dotnet/runtime#56089
Context: dotnet/runtime#57800

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 6.0.100-rc.1.21418.8 to 6.0.100-rc.2.21418.44
* Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.6.21416.1 to 6.0.100-preview.6.21418.3
* Microsoft.NETCore.App.Ref: from 6.0.0-rc.1.21417.1 to 6.0.0-rc.2.21417.16

Update `Xamarin.Android.Net.AndroidMessageHandler` so that additional
properties are specified.  Commit dotnet/runtime@57dd919a updated
`HttpMessageHandler` so that additional properties were no longer
"unsupported" on Android, meaning they needed to exist.  As they
didn't yet exist, the linker emitted warnings:

	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.GetClientCertificateOptions(): No members were resolved for 'get_ClientCertificateOptions'.
	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.GetClientCertificates(): No members were resolved for 'get_ClientCertificates'.
	ILLink : warning IL2037: System.Net.Http.HttpClientHandler.SetClientCertificateOptions(ClientCertificateOption): No members were resolved for 'set_ClientCertificateOptions'.

We need to add the missing members that are now expected:

	partial class AndroidMessageHandler {
	    public ClientCertificateOption ClientCertificateOptions { get; set; }
	    public X509CertificateCollection ClientCertificates { get; set; }
	    // …
	}

Note that these new properties *must* be public, as
`Type.GetMethod(string)` is used, which only finds public members.

We can also update `AndroidClientHandlerTests.cs` and remove the
`catch(TargetInvocationException)` blocks added in 1e5bfa3, as
dotnet/runtime#56089 has been fixed.

Begin ignoring `AotTests.NoSymbolsArgShouldReduceAppSize()`, as it
started failing.  See dotnet/runtime#57800.

Finally, `SslProtocols.Tls13` is only valid on API-29+.
Update `AndroidMessageHandler.SslProtocols` so that
`SslProtocols.Tls13` is only used on appropriate platforms.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@steveisok steveisok

Labels
area-System.Net.Http os-android os-ios Apple iOS
Projects
None yet
Milestone
6.0.0
2 participants
@steveisok @jonathanpeppers