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

Fixed iOS bug where resend = no body #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GeoffArmstrong
Copy link

On iOS, copying the stream a second time resulted in no body because the
stream position would have been advanced. Using ReadAsByteArray prevents
this problem, it's more parallel with Android, and the implementation is
simpler overall.

On iOS, copying the stream a second time resulted in no body because the
stream position would have been advanced. Using ReadAsByteArray prevents
this problem, it's more parallel with Android, and the implementation is
simpler overall.
@anaisbetts
Copy link
Owner

Hm, both the old version and the new version don't check the cancellation token, and copying the request stream could be a long process. Can you make that ReadAsByteArray cancelable? You probably have to copy-paste the implementation and add cancellation.

@GeoffArmstrong
Copy link
Author

I'm just trying to fix a bug I ran into when implementing retry logic on transient failures. Resending a message worked fine in Android, but it was missing the body of the request in iOS. Since the Android code used ReadAsByteArray I figured it was a good solution.

I'm not sure exactly what you want in terms of respecting cancellation tokens, as I'm not an expert at writing async code. Perhaps you could just merge it in and make the changes you're intending?

@anaisbetts
Copy link
Owner

@GeoffArmstrong Basically, instead of using the built-in ReadAsByteArrayAsync, create a new method:

public Task<byte[]> ReadAsByteArrayCancellable(this HttpContent content, CancellationToken ct)
{
    // Implement me, possibly using the code from https://github.com/mono/mono/blob/effa4c07ba850bedbe1ff54b2a5df281c058ebcb/mcs/class/System.Net.Http/System.Net.Http/HttpContent.cs#L158
}

then it's as easy as just replacing your ReadAsByteArrayAsync with your version

@GeoffArmstrong
Copy link
Author

Do you just want to put another cancellationToken.ThrowIfCancellationRequested() at the top of the function then? If we're not rewriting the HttpContent descendants then I don't see how we'd be able to cancel the content copies mid-copy.

@anaisbetts
Copy link
Owner

Do you just want to put another cancellationToken.ThrowIfCancellationRequested() at the top of the function then?

No, it's gotta be in the body of the ReadAsByteArrayAsync, we actually want to interrupt reading the stream

@anaisbetts anaisbetts force-pushed the master branch 2 times, most recently from 7de225e to 63dfb1a Compare October 16, 2014 19:21
@GeoffArmstrong
Copy link
Author

So I updated to version 2.3, and it still has this issue. If I resend the same HttpRequestMessage (because the last time I sent it it timed out), the content doesn't get sent properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants