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

Allow request timeout to be set #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MattMinke
Copy link
Contributor

While unlikely its still possible that a request will take longer than desired and currently there is no way to control the request timeout duration. This pull request adds the ability for timeout to be set. It also adds the lays the ground work for other properties like Expect100Continue, Proxy, and UseNagleAlgorithm to be set as desired by the consumer of the SDK.

@MattMinke
Copy link
Contributor Author

Any update on this being merged ?

@macdonaldster
Copy link

Hi, any chance we will be getting this PR accepted? I am really interested in being able to set this timeout.

@Minx-SigEp - are you using this in production somewhere? Has it been working well?

@MattMinke
Copy link
Contributor Author

@macdonaldster yes I am using it in production and it has been working well for us. Its been awhile since I added this pull request but I have not experienced any issues.

@caviyacht
Copy link

Any update on getting this in? We are in need of this as of the most recent Avalara outage that just happened today. Looking at the latest commit for this on March 5th, for some reason it was hardcoded to be 20 minutes!! That is very extreme.

@macdonaldster
Copy link

macdonaldster commented May 1, 2020

Any update on getting this in? We are in need of this as of the most recent Avalara outage that just happened today. Looking at the latest commit for this on March 5th, for some reason it was hardcoded to be 20 minutes!! That is very extreme.

Hi, I wanted to let you know I was having a very hard time with this as it seemed unreasonable to have a timeout like that with no way to control it. I worked around it by wrapping the call to Avalara in a thread context that would abandon after 3s. I have a local table of tax rates we update monthly and fall back to when we cannot contact Avalara. Since implementing this, we have tripped the timeout many, many times.


 // here, call Avalara but don't wait longer than the timeout (MSEC)
var task = Task.Run(() => AvalaraTaxQueries.TaxOrderRequest(...)));
if (task.Wait(TimeSpan.FromMilliseconds(AvalaraTaxQueries.AvalaraApiTimeout)))
{
	orderTaxData = task.Result as ...; 
}
else
{
	Log.Error("Avalara request took longer than  AvalaraTaxQueries.AvalaraApiTimeout: " + AvalaraTaxQueries.AvalaraApiTimeout + " msec so using DB fallback.");
	... 
}


@caviyacht
Copy link

@macdonaldster , I did think of taking that route as well, but I may have to go the route of reflection. We shouldn't have to do this though.

public void SetAvaTaxClientHttpClientTimeout(int milliseconds)
{
    var type = typeof(AvaTaxClient);
    var field = type.GetField("_client", BindingFlags.Static | BindingFlags.NonPublic);

    var httpClient = field.GetValue(null) as HttpClient;

    httpClient.Timeout = TimeSpan.FromMilliseconds(milliseconds);
}

As for the offline table, I still need to figure that out. I feel like there should be a way to feed the SDK offline data; but maybe I'm just dreaming.

@MattMinke
Copy link
Contributor Author

I have merged upstream master back into AllowRequestTimeToBeSet. This has brought this pull request up to date. It would be nice if we could get this PR accepted.

@jeffjas
Copy link

jeffjas commented Jan 18, 2022

I wanted to push this topic. It is currently possible to set the timeout over UserConfiguration. Unfortunately it is only possible to set the timeout in minutes. This is not very helpful. I dont think that anyone want to wait minutes for a response. It should be possible to set the timeout in milliseconds.

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.

4 participants