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

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

Hi!

Thanks for your review.

In verbal discussion with Georgy he came up with an idea to provide
single error buffer for all curl requests instead of the error buffer
in each request and strdup it if needed.
However, turns out that we can't use single error buffer due to async
curl requests and inconsistent buffer filling: it might be filled in
arbitrary part of the curl request, which means it is inconsistent with
the remaining answer object body. In other words, we might get an
answer from one request and an error buffer from the other asynchronous
request, where it was already filled for some reason.
Therefore, our only option seems to be an error buffer in each request,
which doesn't seem very reasonable, as far as we already have some
error info and we can also obtain more info using @param verbose option
of client_object:request() if needed for debug.
As a conclusion for the thoughts above, I decide to exclude this patch
from the v5 of the current patchset and leave it alone until some
serious reason to reconsider this decision will come up.

>Вторник, 29 октября 2019, 2:42 +03:00 от 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).


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 6406 bytes --]