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

Retry failed connections #2

Open
nakchak opened this issue Aug 11, 2021 · 5 comments
Open

Retry failed connections #2

nakchak opened this issue Aug 11, 2021 · 5 comments

Comments

@nakchak
Copy link

nakchak commented Aug 11, 2021

Carrying on discussion from kamailio/kamailio#2820

Unfortunately my rust is non existent so i don't think i will be able to help much other than discuss ideas...

A retry mechanism would be really handy in case of transient network and server issues.

I have modified the ruxc module to have a basic retry mechanism as follows:

	ruxc_http_post(&v_http_request, &v_http_response);
	
	//Retry logic
	//If attempts < configured retries and return code is < 0 but > -20 [libruxc null url return code] or has a response code between 500-699
	int attempt;
	attempt = 0;
	while(attempt < _ruxc_http_retry && 
	  ((v_http_response.retcode < 0 && v_http_response.retcode > -20) || 
	  (v_http_response.rescode > 499 && v_http_response.rescode < 699)){
		attempt++;
		LM_DBG("Retrying Request Attempt: %d - Response Code: %d - Return Code: %d - URI: %s\n",
		  attempt, v_http_response.retcode, v_http_response.rescode, v_http_request.url);
		ruxc_http_post(&v_http_request, &v_http_response);
	}

Which seems to work nicely and protects against the issues openssl seems to be having on ubuntu 18.04

In order to make this logic as unopinionated as possible there should be an optout mechanism so you could specify that a request shouldnt retry if retries are enabled. I would propose that the least intrusive way of doing this would be to use a "X-" http header to control participation. The header name could be a configurable value with a sensible default of X-RUXC-NORETRY, the presence of the configured header name in a request would disable the retry logic, the value would be irrelevant.

There is also no reason why the configured retry attempts couldnt also be overriden with a http header included in the request.

i.e. if 3 retries are configured globally, then for a given unreliable destination you could override the number of attempts by including an X-RUXC-RETRYATTEMPTS in the specific request.
This would allow simple setting of defaults through mod params if using kamailio, and per request overrides if you add the headers in kamailio config, or any other client to libruxc

@miconda
Copy link
Owner

miconda commented Aug 11, 2021

I would rather add a new field in the request structure, rather that relying on headers, because then we have to specify which ones should be propagated to http message or skipped. The structure is used per function call, so should be easy to do requests with or without retry.

See the structure in the c header file:

The library is at its very beginning, so adding new fields is ok (or use the exiting by-now-unused flags field), kamailio module is also not released.

@nakchak
Copy link
Author

nakchak commented Aug 11, 2021

I would only view headers as a secondary control mechanism, just to be able to override values at runtime when used with kamailio, i.e. have a pv set the retry attempts, but thats really a module feature not a library feature.

Ive been meaning to have a look at rust for a while now, so this has given me all the excuse i need to spend the afternoon having a learn

@miconda
Copy link
Owner

miconda commented Aug 11, 2021

I made a first attempt of retrying the http request by reusing the http agent. You can try with latest dev version of libruxc and set the retry field in the C code:

If works as expected, then a modparam can be added to kamailio module to set it as a global option, also look for a way to control it per https request done in kamailio.cfg routing blocks.

@nakchak
Copy link
Author

nakchak commented Aug 12, 2021

Initial testing if going well, no connection failures logged in a timespan that would usually log one.

Where is the best place to discuss module functionality here or on the kamailio repo?

For enabling config block/per request control i guess it depends on how you see ruxc developing, would the plan be to add more rust exports apart from HTTP based functionality?

As mentioned for HTTP use i have a preference for HTTP headers (or a control string) used as an adhoc control mechanism as my dev team are used to providing various control headers to some api's we use and keeps things pretty extensible, i.e. if basic auth is added to the lib, then it wouldn't be hard to add to add per request credentials using headers, similarly if download handling was added then an output path could be supplied by header.

My hesitancy in adding arguments to the basic ruxv_http_get() signature is that it could descend into the positional hell that is asterisk dialplan apps and functions, while ruxc_http_get(uri, headers, pv) is concise it could grow to ruxc_http_get(url, headers, pv, username, password, retry, debug, outputPath,....), i guess a control argument could be provided and is handled similarly to attributes in dispatcher (; separated list of keywords and values) which would work as well, either way some string parsing would be required.
Which then loops the discussion back around to use headers for control or an attribute string, and as attribute strings are already widely used by dispatcher that would be a more consistent approach, that another ad hoc control mechanism.

so having talked my self around i would propose this:

ruxc_http_get(url, headers, pv, requestOptions)
ruxc_http_post(url, body, headers, pv, requestOptions)

with requestOptions looking like this:
debug={>=1,yes,true};retries={0-10};reuseConnection={0,1,2},tlsAccept={0,1},timeout={0-10000}

Another thing that occurs to me is should the lib have configurable max values which cant be overriden by requestOptions?, as it is a blocking process and if someone sets a 60000ms timeout for example that would almost certainly break a call attempt that invoked it. Or should user protection live higher up in the module, as the short timeout requirement (we have a 100ms max response time for any service which can block on dialog progress) for example is peculiar to VoIP user experience and not general purpose GETting and POSTing....

@miconda
Copy link
Owner

miconda commented Aug 13, 2021

Using headers is suitable for passing info between applications, being done via the protocol, but for controlling a lib I consider that it has to be done via parameters/structure fields.

In this case, mixing headers that should be set to HTTP request with headers that should be used only to control libruxc behaviour makes it confusing. Then it has to be defined a way to specify what headers are for control and should not be passed further to http layer, bringing more complexity. I started from the beginning with two structures, one for defining the http request and the other one for http response, so the functions prototypes stay the same, but new options can be passed by adding fields to structures.

For the kamailio module, the idea was to have something close to a drop-in replacement for http_client module. It has the option to defines connection attributes via modparam or in a configuration file:

The idea to provide the options via a new parameter for kamailio functions seems a good alternative as well.

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

No branches or pull requests

2 participants