NSUrlSessionHandler: Rewind request content to the start before sending #16341

wcoder · 2022-10-13T23:02:23Z

mandel-macaque · 2022-10-14T16:01:44Z

Thanks for this I was looking into this. We need to add a test case before we land this.

wcoder · 2022-10-15T16:33:49Z

We need to add a test case before we land this.

I agree, can you help me with this:
Is any documentation or guides about tests in this repository? How to setup test environments (add/run tests)?

As I see the right place to add tests is here: ./tests/monotouch-test/System.Net.Http/MessageHandlers.cs. Is It so?

I tried to run ./tests/tests.sln from VS4mac and I can't even compile the solution. Could you help with this?

mandel-macaque · 2022-10-17T15:53:02Z

@wcoder I will take care of this and will push a test example to this branch. We can later agree on my tests :)

mandel-macaque · 2022-10-24T18:23:21Z

/azp run

azure-pipelines · 2022-10-24T18:23:49Z

Azure Pipelines successfully started running 1 pipeline(s).

wcoder · 2022-10-24T18:28:16Z

@microsoft-github-policy-service agree

dotMorten · 2022-10-24T18:28:17Z

src/Foundation/NSUrlSessionHandler.cs

+				if (stream.CanSeek)
+					stream.Seek(0, SeekOrigin.Begin);


Don't assume that the thing I want to send is from the beginning of the stream. If I do a partial upload of a file (ie resumed upload), I'd have an offset in my filestream. Instead rewind to where the request was originally meant to start from, and definitely only on an automated retry.

I'm not sure that described behavior has been supported before. Do you have an example?

As I can see, the current implementation of NSUrlSessionHandler uses simple NSURLSessionDataTask under the hood. If you need specific upload a file you should be sure that the native layer supports this behavior.

There is NSURLSessionUploadTask in iOS for upload but we can't simply use it without reworking the native handler.

If you need specific upload a file you should be sure that the native layer

I don't see why I need to do that. The normal http handlers does this just fine. Regardless, you're posting some stream,. and it's wrong to assume it's from the beginning of that stream, and that was all I was pointing out.

If that is true then the behaviour in the android equivalent of this class is wrong. See: AndroidMessageHandler.cs

if (stream.CanSeek) stream.Seek (0, SeekOrigin.Begin);

Yup it would seem so

I think doing the same thing Android does is good enough for now, it's still better than the previous behavior.

mandel-macaque · 2022-10-24T18:45:26Z

I'm going to block this since I want to get input from the dotnet team, I'll me looking at how the socket handler is implemented too.

tests/monotouch-test/System.Net.Http/MessageHandlers.cs

stephen-hawley · 2022-10-25T14:59:15Z

👀

wcoder · 2022-11-23T17:31:09Z

@mandel-macaque Hi, Do you have some updates?

wcoder · 2023-01-23T09:22:11Z

I'm going to block this since I want to get input from the dotnet team, I'll me looking at how the socket handler is implemented too.

@mandel-macaque Do you have any updates?

DuncanMT · 2023-05-12T21:48:44Z

Any news on this? This breaks the Polly retry library.

mandel-macaque · 2023-06-27T16:53:26Z

@DuncaMT bumping this in my todo, I'll take care of it ASAP.

wcoder · 2023-11-20T20:43:23Z

Hi @mandel-macaque, sorry to bother you. May you have some time to review this PR?

microsoft-github-policy-service · 2024-02-26T08:11:28Z

Hi @{issueAuthor}. Due to inactivity, we will close this pull request in 7 days.

microsoft-github-policy-service · 2024-02-26T08:11:29Z

Hi @{issueAuthor}. This pull request was closed due to inactivity. Please let us know if you'd like to reopen it.

wcoder · 2024-02-26T09:59:05Z

@mandel-macaque Could you please reopen this PR?

@microsoft-github-policy-service bot looks too buggy

tipa · 2024-02-27T07:59:23Z

Would love to see some progress on this PR, since now the AndroidMessageHandler is also being fixed that already had stream rewind logic:
https://github.com/xamarin/xamarin-android/blob/a82ac4e5608b9745916db320f4ff36921d4d6dee/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L543-L561.
Once both PRs are merged and released, I could finally simplify my HTTP retry logic :)

github-actions · 2024-02-27T14:31:39Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

dotMorten · 2024-02-27T18:24:46Z

Agreed. This would be some awesome cleanup that'll improve our retry logic as well and save some ugly byte array copying to ensure things are retriable

rolfbjarne · 2024-02-28T07:09:17Z

/azp run

azure-pipelines · 2024-02-28T07:09:39Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2024-02-28T08:44:13Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 21a922c5fe199320f9be26b270de68251e2294bf [PR build]

vs-mobiletools-engineering-service2 · 2024-02-28T08:50:00Z

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.