added rewritten NSUrlSessionHandler that handles memory better

[nberardi]

With the original ModernHttpClient version of NSUrlSessionHandler it had 3 problems.

1. The data in the request was loaded into memory using a byte array. code ref
2. The data in the response was loaded into managed memory from unmanaged memory. And not properly disposed of. code ref
3. The await response is returned before the stream is available, which is not how the .NET framework works. When the await response is returned in .NET the stream is available for reading. code ref

This pull request does the following the correct the above.

1. Wraps a managed stream in an unmanaged stream and sends it to the request as a stream instead of byte array.
2. Now directly uses NSData instead of transitioning to a managed byte array. And disposes of the NSData after fully read.
3. The await response is now returned after the first piece of data is returned or the request is completed.

Please note this is a complete rewrite, which is why I didn't try to commit it to the ModernHttpClient repository. I am willing to move it some where else if it is more appropriate.

[dnfclas]

Hi @nberardi, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@nberardi, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[spouliot]

Hello Nick, it was nice meeting you at Evolve and I wanted to email you about this :-)

There's a few things to solve, and might takes a few iterations, but your results were impressive in memory usage (lower and consistent) so this is something I'd like to move forward. I'll add some initial comments asap, thanks!

[spouliot]

This was based on the ModernHttpClient source code so it must retain it's copyright notice and your own should be added too.

[nberardi]

I will add them.

[spouliot]

Beside the few initial comments (I did not test it locally yet) this will need to be updated to match our coding guideline (http://www.mono-project.com/community/contributing/coding-guidelines/) but that's fairly minor and can wait for further testing/comments. Thanks again!

[nberardi]

I will work on the changes above and contribute. It might be a few days until I get to it, because I am in Disney World until Saturday. I had a down day today because, it was raining and we decided to stay at the hotel all day. :)

I also was able to solve the upload memory issue too, I didn't have a proof for you at the Evolve Conference, but I created a demo app to prove both upload and download to myself this afternoon.

I am still working on a further operation that I will commit as a different pull request. Because it further optimizes the use of NSData by not causing the memory to realign. However, this is a current problem with both ModernHttpClient and this version.

Because the NSData object is often pieced together from a number of different data objects, whenever possible, use NSData’s enumerateByteRangesUsingBlock: method to iterate through the data rather than using the bytes method (which flattens the NSData object into a single memory block).

This delegate method may be called more than once, and each call provides only data received since the previous call. The app is responsible for accumulating this data if needed. source link

[spouliot]

Great, I'm looking forward to the updates :) Have a nice time at Disney World!

[nberardi]

Made the changes recommended by @spouliot above. I will squash the changes into one commit after everything is signed off.

[rolfbjarne]

This looks redundant, since it's just calling the base method. Can't this override just be removed?

[nberardi]

are you talking about Dispose? I think I forgot to implement the disposing of the inflight data. Thanks for the catch.

[rolfbjarne]

Yes, I'm talking about the Dispose method.

[nberardi]

With the latest commit, I am able to wrap NSInputStream only using public methods.

[nberardi]

Has there been any movement on getting this approved?

[spouliot]

Consider it dev approved, i.e. everything looks good, there's no further comments. However it's a large (impact) change and most of our networking tests are done by QA (not on our development bots).

I'll have QA test a branch with your changes before merging into master (standard policy for any large change). The exact dates depends on QA availability (currently focusing on getting C7 to stable).

Can you rebase/squash your changes so I can a bot to build a package based on the PR ? thanks!

[nberardi]

@spouliot everything was squashed and rebased against the latest commits upstream. No problem on the timing, if anything comes up let me know, I will be happy to work through the issues.

[spouliot]

Thanks! We now have a lane that will produce the package fro QA and I'll chat with them on Monday about testing this. Have a nice weekend!

[spouliot]

for internal tracking https://trello.com/c/1zWlEQbd/40-xi-xm-new-nsurlsessionhandler

[spouliot]

The QA tests failed. While the old files (from ModernHttpClient) were removed from the build it looks like the new NSUrlSessionHandler.cs was not added.

As such the bots built a package without including the new code and the tests either failed (specific checks for NSUrlSessionHandler) or worked, because the code reverted to use the managed implementation if it could not find the specific type.

[nberardi]

I apologize I don't know a lot about how your bots build packages. Do you have some documents that I can look at so that I can ensure I have everything configured correctly?

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On May 20, 2016, at 6:03 AM, Sebastien Pouliot [email protected] wrote:

The QA tests failed. While the old files (from ModernHttpClient) were removed from the build it looks like the new NSUrlSessionHandler.cs was not added.

As such the bots built a package without including the new code and the tests either failed (specific checks for NSUrlSessionHandler) or worked, because the code reverted to use the managed implementation if it could not find the specific type.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[spouliot]

No need to apologize :) we missed it during the review. You're likely testing the new source file as part of your app/test code.

Just like some files were removed from src/Makefile this new file you're adding, src/Foundation/NSUrlSessionHandler.cs must be added (at the same places) to the code gets compiled in the right assemblies (which is different for XI and XM due to historical reasons, e.g. it's located in System.Net.Http.dll for XI).

I'll double check if we have (pre-QA) tests that check for this (missing type). Every PR are now built on new bots and we'll soon enable a subset of our tests on them. That should give good/bad feedback (for such cases) a lot faster.

[nberardi]

I added the files into the Makefile in the places they were previously removed. Let me know if there is anything else you see that needs to go into the commit.

[spouliot]

build

[nberardi]

@spouliot I know you are in the later stages of cycle 8 now. Let me know when you have some time to discuss next steps for this pull request for a cycle 9 release.

[spouliot]

@nberardi I asked @mandel-macaque to follow up on the PR and QA for our cycle9 (planned to be shorter than usual).

[mandel-macaque]

Hello, @nberardi I'm trying to land this changes in our new development cycle yet we have identified the following bug:

401 code not returned when using NSURLSession HTTPClient handler.

I am working on a fix for it but I was wondering if you do have one and you have not yet pushed it.

[mandel-macaque]

FYI I'm working on the branch new-url-session-handler

[nberardi]

Do you have a repo application?

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Sep 27, 2016, at 7:34 AM, Manuel de la Pena [email protected] wrote:

Hello, @nberardi I'm trying to land this changes in our new development cycle yet we have identified the following bug:

401 code not returned when using NSURLSession HTTPClient handler.

I am working on a fix for it but I was wondering if you do have one and you have not yet pushed it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[mandel-macaque]

@nberardi There is a sample attached to the bug report.

[nberardi]

If I remember correctly authentication is handled through a different callback. I will take a look in this office this morning.

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Sep 28, 2016, at 4:32 AM, Manuel de la Pena [email protected] wrote:

@nberardi There is a sample attached to the bug report.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[nberardi]

@mandel-macaque I don't appear to have access to the attachments in bugzilla.

[nberardi]

@mandel-macaque I created a simple example to get a 401 response, I cannot confirm the bugzilla bug.

        private const string UnathorizedResponse = "http://httpstat.us/401";

        private Task<bool> Run (HttpMessageHandler handler, string requestUrl, HttpStatusCode responseCode)
        {
            return Task.Run (() => TestResponseStatus (handler, requestUrl, responseCode));
        }

        private async Task<bool> TestResponseStatus (HttpMessageHandler handler, string requestUrl, HttpStatusCode responseCode)
        {
            var http = new HttpClient (handler);
            var response = await http.GetAsync (requestUrl);

            var status = response.StatusCode;
            if (response.StatusCode == responseCode)
                return true;
            else
                return false;
        }

        [Export("startDownloadUsingNewNetworkPressed:")]
        private void startDownloadUsingNewNetworkPressed(NSObject sender)
        {
            if (Run (new Test.NSUrlSessionHandler (), UnathorizedResponse, HttpStatusCode.Unauthorized).Result == true)
                Console.WriteLine ("YAH");
        }

I get back true each time.

[nberardi]

@mandel-macaque I made another commit to my PR, that added a couple ConfigureAwait(false) statements that I originally forgot to add. I found this when I was doing a code review for the 401 issue.

[mandel-macaque]

@nberardi Great, I'm going to land this on #932 since I did a merge with master and fix al the diff merge issues.

Once we have that we (mostly I) can look at the 401 since it is a bug that happens in the old handler and could be related to old code that has nothing to do with your work :)

Once the PR is merged I'll close this one and @spouliot once and we can move fwd.

[mandel-macaque]

@nberardi Happy to say that we have merged your code in master!!!! Thx for the awesome work. Will be closing this PR since it is not longer needed :)

Again, thanks a lot for the work.