<HTML><BODY><span style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Hi!</span><br style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;"><br style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;"><span style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Thanks for your review.</span><br><br><p>In verbal discussion with Georgy he came up with an idea to provide<br>single error buffer for all curl requests instead of the error buffer<br>in each request and strdup it if needed.<br>However, turns out that we can't use single error buffer due to async<br>curl requests and inconsistent buffer filling: it might be filled in<br>arbitrary part of the curl request, which means it is inconsistent with<br>the remaining answer object body. In other words, we might get an<br>answer from one request and an error buffer from the other asynchronous<br>request, where it was already filled for some reason.<br>Therefore, our only option seems to be an error buffer in each request,<br>which doesn't seem very reasonable, as far as we already have some<br>error info and we can also obtain more info using @param verbose option<br>of client_object:request() if needed for debug.<br>As a conclusion for the thoughts above, I decide to exclude this patch<br>from the v5 of the current patchset and leave it alone until some<br>serious reason to reconsider this decision will come up.</p><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Вторник, 29 октября 2019, 2:42 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<div id="style_15723061242064753187_BODY">Now I don't sure whether we should add the field: the cost is increased<br>
memory footprint (it is megabytes even to 10k requests in the fly, but<br>
this will spoil caches) and so performance. Don't sure how significant<br>
cons and pros are.<br>
<br>
Anyway, my comments and thoughts are below.<br>
<br>
WBR, Alexander Turenko.<br>
<br>
> http: enrich httpc_request with curl error message buffer<br>
Try to fit the commit header into 50 symbols and concentrate on changes<br>
that are visible for a user. How about this header?<br>
<br>
| httpc: give more information is case of an error<br>
<br>
> When processing curl request error code, we are collecting error<br>
> message using curl_easy_strerror. Now we are providing more info<br>
> in errmsg, which is obtained using curl_easy_setopt with<br>
<br>
I would say: in 'errmsg' field of a response object.<br>
<br>
> CURLOPT_ERRORBUFFER option. Test case to confirm it is added.<br>
> <br>
> Closes #4578<br>
> <br>
> @TarantoolBot document<br>
> Title: http: new field in client_object:request return table<br>
<br>
http -> httpc<br>
<br>
`client_object:request` is not valid Lua syntax: better use<br>
`client_object:request()`.<br>
<br>
> Update the documentation for client_object:request method to reflect<br>
> new field errmsg in return table. It contains extended curl request<br>
> error message obtained using CURLOPT_ERRORBUFFER option.<br>
<br>
An example about, say, your case with unsupported content encoding would<br>
shed light on why this may be needed, when a request object already<br>
contains 'reason' field.<br>
<br>
> diff --git a/src/httpc.h b/src/httpc.h<br>
> index f710b3d13..145b59929 100644<br>
> --- a/src/httpc.h<br>
> +++ b/src/httpc.h<br>
> @@ -105,6 +105,11 @@ struct httpc_request {<br>
> int status;<br>
> /** Error message */<br>
> const char *reason;<br>
> + /**<br>
> + * Buffer for error message obtained using<br>
> + * CURLOPT_ERRORBUFFER option.<br>
> + */<br>
> + const char errmsg[CURL_ERROR_SIZE];<br>
<br>
It is indented using whitespaces rather then tabulation.<br>
<br>
I'm tentative about placing 256 bytes buffer right in the structure.<br>
However the whole structure is allocated with mempool_alloc(), so maybe<br>
it is okay.<br>
<br>
Also I don't sure whether it is okay to add 256 bytes of allocated<br>
memory per request unconditionally: maybe we should provide an option to<br>
disable it for applications that have tight SLA on a response processing<br>
time.<br>
<br>
I would give Georgy necessary context (what and how we allocate in our<br>
http client now) and ask for his opinion on how and where allocate the<br>
buffer.<br>
<br>
BTW, I think that it is good to provide the field by default (in case of<br>
an error) to allow to find a cause of an error faster during<br>
development.<br>
<br>
Maybe adding this field is not the good idea if it is not cheap and need<br>
to be switched on/off case by case: AFAIR the same information can be<br>
obtained using 'verbose' flag.<br>
<br>
It would be good to catch Georgy's opinion on that too.<br>
<br>
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c<br>
> index 6ed4eb788..9fb8e4801 100644<br>
> --- a/src/lua/httpc.c<br>
> +++ b/src/lua/httpc.c<br>
> @@ -335,6 +335,10 @@ luaT_httpc_request(lua_State *L)<br>
> lua_pushstring(L, req->reason);<br>
> lua_settable(L, -3);<br>
> <br>
> + lua_pushstring(L, "errmsg");<br>
> + lua_pushstring(L, req->errmsg);<br>
> + lua_settable(L, -3);<br>
<br>
I think we should add the field only when an error occurs. Now we add it<br>
for successful responses and it looks a bit confusing.<br>
<br>
More on that:<br>
<br>
> For earlier versions if an error code was returned but there was no<br>
> error detail then the buffer is untouched. <br>
> <a href="https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html" target="_blank">https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html</a><br>
<br>
We should handle this case and set a buffer to an empty string before a<br>
request and verify that a code is not CURLE_OK afterwards (see the<br>
example by the link above).<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>-- <br>Ilya Kosarev<br></BODY></HTML>