alexrainman / ModernHttpClient

ModernHttpClient
MIT License
126 stars 28 forks source link

Timeout seems to be ignored on Android #4

Closed digital-synapse closed 5 years ago

digital-synapse commented 6 years ago

httpClient.Timeout = msTimeout == -1 ? defaultTimeout : TimeSpan.FromMilliseconds(msTimeout);

//...

using (webResponse = await httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false))

// Timeout was ignored. if placing a breakpoint in OkHttpNetworkHandler.cs you can see that Timeout is actually null somehow even though it was previously set.

alexrainman commented 6 years ago

Wrong. Use NativeMessageHandler Timeout property.

digital-synapse commented 6 years ago

Ahh i see, Somehow setting httpClient.Timeout worked with the original ModernHttpClient. As you say, setting on the handler instead solves the issue. Thank you!

Johnderate commented 5 years ago

This doesn't work for me, the timeout is still 10 seconds, no matter what I set.

This is my code:

/// <inheritdoc />
/// <summary>
/// </summary>
/// <param name="path"></param>
/// <param name="method"></param>
/// <param name="body"></param>
/// <param name="ct"></param>
/// <param name="accessToken">Use a custom access token e.G. send app infos</param>
/// <param name="timeout">Timeout for call</param>
/// <returns>HttpResponseMessage</returns>
public async Task<HttpResponseMessage> CallApiAsync(string path,
    HttpMethod method = null,
    string body = null,
    CancellationToken ct = default(CancellationToken),
    string accessToken = null,
    TimeSpan timeout = default(TimeSpan))
{
    var stopwatch = Stopwatch.StartNew();
    HttpResponseMessage response = null;

    if (timeout == default(TimeSpan))
        timeout = TimeSpan.FromSeconds(60);

    try
    {
        _telemetryService?.Log($"BackendConnectorService.CallApiAsync({path})");

        if (method == null)
        {
            method = HttpMethod.Get;
        }

        // call backend service and get json content
        using (var client = new HttpClient(new NativeMessageHandler(false, false, new NativeCookieHandler()) {Timeout = timeout}))
        {
            var request = new HttpRequestMessage(method, new Uri(path));

            if (accessToken != null)
                // TryAddWihtoutValidation is normaly used when you don't want the standard format
                client.DefaultRequestHeaders.TryAddWithoutValidation("Authorization", accessToken);
            // This would be the "normal" way to do it
            //client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(accessToken);
            else
                client.InitializeHeader(_settingsService);

            var ci = _cultureService.GetCurrentCultureInfo();
            request.Headers.Add("ui-culture", ci);

            request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

            if (body != null)
                request.Content = new StringContent(body, Encoding.UTF8, "application/json");
            else if (method == HttpMethod.Post || method == HttpMethod.Put)
                request.Content = new StringContent("");

            response = await client.SendAsync(request, ct).ConfigureAwait(false);

            if (response.StatusCode != HttpStatusCode.OK)
            {
                var content = await response.Content.ReadAsStringAsync();
                _telemetryService?.Log($"Response Status not OK\nBody content: {content}");
            }
        }

        ct.ThrowIfCancellationRequested();

        return response;
    }
    catch (Exception ex)
    {
        _telemetryService?.TrackException(ex);
        throw;
    }
    finally
    {
        stopwatch.Stop();
        if (response != null)
            _telemetryService?.TrackRequest(path, DateTime.Now, stopwatch.Elapsed, response.StatusCode.ToString(), response.IsSuccessStatusCode);
    }
}
alexrainman commented 5 years ago

Android or iOS?

Johnderate commented 5 years ago

This is on Android. The 10 seconds are the standard timeout from okhttp3 I presume.

alexrainman commented 5 years ago

I will try to reproduce this.

adrianknight89 commented 5 years ago

I set the timeout on NativeMessageHandler to 30 secs, and it seems to be working on Android for me.

@alexrainman Do you have any guidance on handling timeout exceptions using your library? When an exception is thrown, I see that the Message property is "timeout" (at least on Android), but there should be a more reliable way of handling this.

Johnderate commented 5 years ago

@adrianknight89 Are you doing anything differently than I am in my example above? (I know its quite a mouthful)

adrianknight89 commented 5 years ago

@Johnderate In my case, I have a single static instance of HttpClient that gets used across the application, and I'm also not setting anything on NativeMessageHandler other than Timeout. I know this is not good practice, but I've defaulted every API to be POST (hence I'm invoking PostAsync instead of SendAsync on the client). You should see if you're having your timeout issues with GET requests or all HTTP methods.

Make sure to update your Nuget packages. I'm also using client.DefaultRequestHeaders.Authorization. Finally, I'm not building an HttpRequestMessage. Things like CultureInfo can be passed as an API parameter that's part of HttpContent.

alexrainman commented 5 years ago

Using a static instance of HttpClient is the right way to do it, otherwise you won't be reusing sockets. I have tried to reproduce this without luck.