-
Notifications
You must be signed in to change notification settings - Fork 564
[Mono.Android, Xamarin.Android.Net.AndroidClientHandler] Fix requests with content #44
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
Conversation
|
Hi @blounty, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
| SSLContext context = SSLContext.GetInstance ("TLS"); | ||
| context.Init (kmf?.GetKeyManagers (), tmf.GetTrustManagers (), null); | ||
| httpsConnection.SSLSocketFactory = context.SocketFactory; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabs. vs. spaces mismatch. Please use tabs.
|
Tests for this should be added to https://github.com/xamarin/xamarin-android/blob/master/src/Mono.Android/Test/Xamarin.Android.Net/AndroidClientHandlerTests.cs. |
|
LGTM - after tabs vs spaces are fixed and tests added. And please squash the commits into one, thanks :) |
|
Hi @blounty, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| ICredentials creds = data.UseProxyAuthentication ? Proxy?.Credentials : Credentials; | ||
| if (creds == null) { | ||
| if (Logger.LogNet) | ||
| if (Logger.LogNet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this indented?
…ork with requests which contains content. Also fixed a race condition which causes the Output stream to not be ready. Fixed issues with calling on the MainThread which is not alowed with the Java HttpURLConnection updating logger incorrect calls adding fixes
|
This still needs unit tests. :-) |
|
@blounty I'm going to accept this seeing that more and more people run into this issue, however please provide the tests whenever you have the chance, thanks :) |
|
I've solved this issue in my fork: danielcaceresm@bd1f0b4 These changes are based on the AndroidClientHandler class before the modifications of this pull request. Other change I've made is to move the connection reference to the response class, this approach is better than holding the last connection in the handler. On the other hand, is pending to implement, dispose the connection if it fails while connecting. If you consider my changes, I can open a new pull request. |
|
@danielcaceresm please open a separete PR |
Running `make all` would fail: make: *** No rule to make target `bin/TestDebug/Java.Interop.Tools.JavaCallableWrappers-Tests.dll', needed by `all'. Stop. This happened because `Java.Interop.Tools.JavaCallableWrappers-Tests.dll` was part of `$(TESTS)`, which is a dependency of the `all` target, but there wasn't a rule to create that file. Add a rule to create `bin/TestDebug/Java.Interop.Tools.JavaCallableWrappers-Tests.dll`.
Updated how content is handled, it seems content was not previously being added.
There also appeared to be a race condition which sometimes caused the connection output stream to no be ready at the time of writing.