Re: [Tarantool-patches] [PATCH v4 3/4] http: enrich httpc_request with curl error message buffer

[Alexander Turenko <alexander.turenko@tarantool.org>]

Now I don't sure whether we should add the field: the cost is increased
memory footprint (it is megabytes even to 10k requests in the fly, but
this will spoil caches) and so performance. Don't sure how significant
cons and pros are.

Anyway, my comments and thoughts are below.

WBR, Alexander Turenko.

> http: enrich httpc_request with curl error message buffer

Try to fit the commit header into 50 symbols and concentrate on changes
that are visible for a user. How about this header?

 | httpc: give more information is case of an error

> When processing curl request error code, we are collecting error
> message using curl_easy_strerror. Now we are providing more info
> in errmsg, which is obtained using curl_easy_setopt with

I would say: in 'errmsg' field of a response object.

> CURLOPT_ERRORBUFFER option. Test case to confirm it is added.
> 
> Closes #4578
> 
> @TarantoolBot document
> Title: http: new field in client_object:request return table

http -> httpc

`client_object:request` is not valid Lua syntax: better use
`client_object:request()`.

> Update the documentation for client_object:request method to reflect
> new field errmsg in return table. It contains extended curl request
> error message obtained using CURLOPT_ERRORBUFFER option.

An example about, say, your case with unsupported content encoding would
shed light on why this may be needed, when a request object already
contains 'reason' field.

> diff --git a/src/httpc.h b/src/httpc.h
> index f710b3d13..145b59929 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -105,6 +105,11 @@ struct httpc_request {
>  	int status;
>  	/** Error message */
>  	const char *reason;
> +	/**
> +	 * Buffer for error message obtained using
> +	 * CURLOPT_ERRORBUFFER option.
> +	 */
> +        const char errmsg[CURL_ERROR_SIZE];

It is indented using whitespaces rather then tabulation.

I'm tentative about placing 256 bytes buffer right in the structure.
However the whole structure is allocated with mempool_alloc(), so maybe
it is okay.

Also I don't sure whether it is okay to add 256 bytes of allocated
memory per request unconditionally: maybe we should provide an option to
disable it for applications that have tight SLA on a response processing
time.

I would give Georgy necessary context (what and how we allocate in our
http client now) and ask for his opinion on how and where allocate the
buffer.

BTW, I think that it is good to provide the field by default (in case of
an error) to allow to find a cause of an error faster during
development.

Maybe adding this field is not the good idea if it is not cheap and need
to be switched on/off case by case: AFAIR the same information can be
obtained using 'verbose' flag.

It would be good to catch Georgy's opinion on that too.

> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index 6ed4eb788..9fb8e4801 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -335,6 +335,10 @@ luaT_httpc_request(lua_State *L)
>  	lua_pushstring(L, req->reason);
>  	lua_settable(L, -3);
>  
> +	lua_pushstring(L, "errmsg");
> +	lua_pushstring(L, req->errmsg);
> +	lua_settable(L, -3);

I think we should add the field only when an error occurs. Now we add it
for successful responses and it looks a bit confusing.

More on that:

> For earlier versions if an error code was returned but there was no
> error detail then the buffer is untouched. 
> https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html

We should handle this case and set a buffer to an empty string before a
request and verify that a code is not CURLE_OK afterwards (see the
example by the link above).