[Tarantool-patches] [PATCH v5 0/3] httpc: add curl accept_encoding option and relevant fixes

[Tarantool-patches] [PATCH v5 0/3] httpc: add curl accept_encoding option and relevant fixes

[Ilya Kosarev]

This patchset introduces curl accept_encoding option.
It also brought up fix for CURLE_WRITE_ERROR processing and addition of
CURLE_BAD_CONTENT_ENCODING case in curl request code processing.

Changes in v2:
- added docbot request
- fixed comments
- enhanced httpc_set_accept_encoding description
- fixed error handling for unsupported encodings

Changes in v3:
- moved error handling for unsupported encodings fix to separate commit
- added error message obtained using CURLOPT_ERRORBUFFER option
- added CURLE_BAD_CONTENT_ENCODING for curl request code processing

Changes in v4:
- fixed commit message in PATCH 2/4
- added test case for error message obtained using CURLOPT_ERRORBUFFER option

Changes in v5:
- commit messages & comments fixed
- excluded ungrounded patch

Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4232-curlopt-accept-encoding
Issues: https://github.com/tarantool/tarantool/issues/4232
        https://github.com/tarantool/tarantool/issues/4579

Ilya Kosarev (3):
  httpc: fix assertion fail after curl write error
  httpc: add curl accept_encoding option
  httpc: handle bad Content-Encoding with curl-7.67.0+

 src/httpc.c       | 23 ++++++++++++++++++-----
 src/httpc.h       | 26 ++++++++++++++++++++++++++
 src/lua/httpc.c   |  5 +++++
 src/lua/httpc.lua |  2 ++
 4 files changed, 51 insertions(+), 5 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v5 1/3] httpc: fix assertion fail after curl write error

[Ilya Kosarev]

After executing curl request we need to process curl_request code. It
might be CURLE_WRITE_ERROR. We had special case for it, which assumed
diagnostic message being set and contained corresponding assert, though
it is incorrect. Better way is to handle it as any other non-standard
event.
It was discovered while adding accept_encoding option. In case of
unknown encoding curl_request code is currently set to
CURLE_WRITE_ERROR and therefore we come to an assert assuming we have
some diagnostics set. However, it is not being set and it is totally
fine. This means we are failing on assert and it is not correct
behavior.

Prerequisites: #4232
---
 src/httpc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/httpc.c b/src/httpc.c
index 8d18b9966..212064080 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -429,16 +429,12 @@ httpc_execute(struct httpc_request *req, double timeout)
 	case CURLE_COULDNT_RESOLVE_PROXY:
 	case CURLE_COULDNT_RESOLVE_HOST:
 	case CURLE_COULDNT_CONNECT:
+	case CURLE_WRITE_ERROR:
 		/* 595 Connection Problem (AnyEvent non-standard) */
 		req->status = 595;
 		req->reason = curl_easy_strerror(req->curl_request.code);
 		++env->stat.failed_requests;
 		break;
-	case CURLE_WRITE_ERROR:
-		/* Diag is already set by curl_write_cb() */
-		assert(!diag_is_empty(&fiber()->diag));
-		++env->stat.failed_requests;
-		return -1;
 	case CURLE_OUT_OF_MEMORY:
 		diag_set(OutOfMemory, 0, "curl", "internal");
 		++env->stat.failed_requests;
-- 
2.17.1

[Tarantool-patches] [PATCH v5 2/3] httpc: add curl accept_encoding option

[Ilya Kosarev]

CURLOPT_ACCEPT_ENCODING option is now supported.
This option enables automatic decompression of HTTP downloads.
See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Closes #4232

@TarantoolBot document
Title: httpc: add curl accept_encoding option
Update the documentation for client_object:request() parameters to
reflect new accept_encoding option.
It enables automatic decompression of HTTP downloads by setting the
contents of the Accept-Encoding header sent in a HTTP request and
enabling decoding of a response when a Content-Encoding header is
received.
This is a request, not an order; the server may or may not do it.
Servers might respond with Content-Encoding even without getting
an Accept-Encoding in the request. Servers might respond with a
different Content-Encoding than what was asked for in the request.
@param accept_encoding specifies what encoding you'd like.
This param can be an empty string which means Accept-Encoding header
will contain all built-in supported encodings. This param can be
comma-separated list of accepted encodings, like: "br, gzip, deflate".
Bundled libcurl supports "identity", meaning non-compressed, "deflate"
which requests the server to compress its response using the zlib
algorithm and "gzip" which requests the gzip algorithm. System libcurl
also possibly supports "br" which is brotli.
See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html
---
 src/httpc.c       | 16 ++++++++++++++++
 src/httpc.h       | 26 ++++++++++++++++++++++++++
 src/lua/httpc.c   |  5 +++++
 src/lua/httpc.lua |  2 ++
 4 files changed, 49 insertions(+)

diff --git a/src/httpc.c b/src/httpc.c
index 212064080..22b54d16a 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -361,6 +361,22 @@ httpc_set_follow_location(struct httpc_request *req, long follow)
 			 follow);
 }
 
+void
+httpc_set_accept_encoding(struct httpc_request *req, const char *encoding)
+{
+/*
+* CURLOPT_ACCEPT_ENCODING was called CURLOPT_ENCODING before
+* libcurl 7.21.6.
+*/
+#if LIBCURL_VERSION_NUM >= 0x071506
+	curl_easy_setopt(req->curl_request.easy, CURLOPT_ACCEPT_ENCODING,
+			 encoding);
+#else
+	curl_easy_setopt(req->curl_request.easy, CURLOPT_ENCODING,
+			 encoding);
+#endif
+}
+
 int
 httpc_execute(struct httpc_request *req, double timeout)
 {
diff --git a/src/httpc.h b/src/httpc.h
index 99fd8fbd4..2fc557107 100644
--- a/src/httpc.h
+++ b/src/httpc.h
@@ -372,6 +372,32 @@ httpc_set_interface(struct httpc_request *req, const char *interface);
 void
 httpc_set_follow_location(struct httpc_request *req, long follow);
 
+/**
+ * Enable automatic decompression of HTTP downloads: set the
+ * contents of the Accept-Encoding header sent in a HTTP request
+ * and enable decoding of a response when a Content-Encoding
+ * header is received.
+ * This is a request, not an order; the server may or may not do
+ * it. Servers might respond with Content-Encoding even without
+ * getting an Accept-Encoding in the request. Servers might
+ * respond with a different Content-Encoding than what was asked
+ * for in the request.
+ * @param req request
+ * @param encoding - specify what encoding you'd like.
+ * This param can be an empty string which means Accept-Encoding
+ * header will contain all built-in supported encodings. This
+ * param can be comma-separated list of accepted encodings, like:
+ * "br, gzip, deflate".
+ * Bundled libcurl supports "identity", meaning non-compressed,
+ * "deflate" which requests the server to compress its response
+ * using the zlib algorithm and "gzip" which requests the gzip
+ * algorithm. System libcurl also possibly supports "br" which
+ * is brotli.
+ * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html
+ */
+void
+httpc_set_accept_encoding(struct httpc_request *req, const char *encoding);
+
 /**
  * This function does async HTTP request
  * @param request - reference to request object with filled fields
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index a8e3e2525..6ed4eb788 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -315,6 +315,11 @@ luaT_httpc_request(lua_State *L)
 		httpc_set_follow_location(req, lua_toboolean(L, -1));
 	lua_pop(L, 1);
 
+	lua_getfield(L, 5, "accept_encoding");
+	if (!lua_isnil(L, -1))
+		httpc_set_accept_encoding(req, lua_tostring(L, -1));
+	lua_pop(L, 1);
+
 	if (httpc_execute(req, timeout) != 0) {
 		httpc_request_delete(req);
 		return luaT_error(L);
diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
index ce9bb9771..3224d8c10 100644
--- a/src/lua/httpc.lua
+++ b/src/lua/httpc.lua
@@ -296,6 +296,8 @@ end
 --          'Location' header that a server sends as part of an
 --          3xx response;
 --
+--      accept_encoding - enables automatic decompression of HTTP downloads;
+--
 --  Returns:
 --      {
 --          status=NUMBER,
-- 
2.17.1

[Tarantool-patches] [PATCH v5 3/3] httpc: handle bad Content-Encoding with curl-7.67.0+

[Ilya Kosarev]

libcurl-7.66.0 and older returns CURLE_WRITE_ERROR when a server
responds with unknown or unsupported Content-Encoding (see [1] and
[2]). This was fixed in future libcurl-7.67.0 and proper
CURLE_BAD_CONTENT_ENCODING code will be returned in this case.

We should process the code in the same way as we do for
CURLE_WRITE_ERROR.

[1]: curl/curl#4310
[2]: curl/curl#4449

Closes #4579
---
 src/httpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/httpc.c b/src/httpc.c
index 22b54d16a..be73e3684 100644
--- a/src/httpc.c
+++ b/src/httpc.c
@@ -446,6 +446,7 @@ httpc_execute(struct httpc_request *req, double timeout)
 	case CURLE_COULDNT_RESOLVE_HOST:
 	case CURLE_COULDNT_CONNECT:
 	case CURLE_WRITE_ERROR:
+	case CURLE_BAD_CONTENT_ENCODING:
 		/* 595 Connection Problem (AnyEvent non-standard) */
 		req->status = 595;
 		req->reason = curl_easy_strerror(req->curl_request.code);
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v5 2/3] httpc: add curl accept_encoding option

[Alexander Turenko]

LGTM.

I change wording a bit before push, commented below.

Pushed to master.

WBR, Alexander Turenko.

On Thu, Nov 07, 2019 at 03:07:15PM +0300, Ilya Kosarev wrote:
> CURLOPT_ACCEPT_ENCODING option is now supported.
> This option enables automatic decompression of HTTP downloads.
> See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

To be honest, I dislike 'HTTP downloads' wording of libcurl
documentation, because for me it suggests that application/octet-stream
mime type is attached to a response. The wording however technically
correct, just confusing for me and others who work more with a browser,
then HTTP directly. I changed it to 'HTTP responses'.

> Closes #4232
> 
> @TarantoolBot document
> Title: httpc: add curl accept_encoding option
> Update the documentation for client_object:request() parameters to
> reflect new accept_encoding option.
> It enables automatic decompression of HTTP downloads by setting the

Same here.

> contents of the Accept-Encoding header sent in a HTTP request and
> enabling decoding of a response when a Content-Encoding header is
> received.
> This is a request, not an order; the server may or may not do it.
> Servers might respond with Content-Encoding even without getting
> an Accept-Encoding in the request. Servers might respond with a
> different Content-Encoding than what was asked for in the request.
> @param accept_encoding specifies what encoding you'd like.
> This param can be an empty string which means Accept-Encoding header
> will contain all built-in supported encodings. This param can be
> comma-separated list of accepted encodings, like: "br, gzip, deflate".
> Bundled libcurl supports "identity", meaning non-compressed, "deflate"
> which requests the server to compress its response using the zlib
> algorithm and "gzip" which requests the gzip algorithm. System libcurl
> also possibly supports "br" which is brotli.
> See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Separated paragraphs with empty lines for readability.

> ---
>  src/httpc.c       | 16 ++++++++++++++++
>  src/httpc.h       | 26 ++++++++++++++++++++++++++
>  src/lua/httpc.c   |  5 +++++
>  src/lua/httpc.lua |  2 ++
>  4 files changed, 49 insertions(+)
> 
> diff --git a/src/httpc.c b/src/httpc.c
> index 212064080..22b54d16a 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -361,6 +361,22 @@ httpc_set_follow_location(struct httpc_request *req, long follow)
>  			 follow);
>  }
>  
> +void
> +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding)
> +{
> +/*
> +* CURLOPT_ACCEPT_ENCODING was called CURLOPT_ENCODING before
> +* libcurl 7.21.6.
> +*/
> +#if LIBCURL_VERSION_NUM >= 0x071506
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_ACCEPT_ENCODING,
> +			 encoding);
> +#else
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_ENCODING,
> +			 encoding);
> +#endif
> +}
> +
>  int
>  httpc_execute(struct httpc_request *req, double timeout)
>  {
> diff --git a/src/httpc.h b/src/httpc.h
> index 99fd8fbd4..2fc557107 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -372,6 +372,32 @@ httpc_set_interface(struct httpc_request *req, const char *interface);
>  void
>  httpc_set_follow_location(struct httpc_request *req, long follow);
>  
> +/**
> + * Enable automatic decompression of HTTP downloads: set the

Same here.

> + * contents of the Accept-Encoding header sent in a HTTP request
> + * and enable decoding of a response when a Content-Encoding
> + * header is received.
> + * This is a request, not an order; the server may or may not do
> + * it. Servers might respond with Content-Encoding even without
> + * getting an Accept-Encoding in the request. Servers might
> + * respond with a different Content-Encoding than what was asked
> + * for in the request.
> + * @param req request
> + * @param encoding - specify what encoding you'd like.
> + * This param can be an empty string which means Accept-Encoding
> + * header will contain all built-in supported encodings. This
> + * param can be comma-separated list of accepted encodings, like:
> + * "br, gzip, deflate".
> + * Bundled libcurl supports "identity", meaning non-compressed,
> + * "deflate" which requests the server to compress its response
> + * using the zlib algorithm and "gzip" which requests the gzip
> + * algorithm. System libcurl also possibly supports "br" which
> + * is brotli.
> + * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Separated paragraphs with empty lines too.

> + */
> +void
> +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding);
> +
>  /**
>   * This function does async HTTP request
>   * @param request - reference to request object with filled fields
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index a8e3e2525..6ed4eb788 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -315,6 +315,11 @@ luaT_httpc_request(lua_State *L)
>  		httpc_set_follow_location(req, lua_toboolean(L, -1));
>  	lua_pop(L, 1);
>  
> +	lua_getfield(L, 5, "accept_encoding");
> +	if (!lua_isnil(L, -1))
> +		httpc_set_accept_encoding(req, lua_tostring(L, -1));
> +	lua_pop(L, 1);
> +
>  	if (httpc_execute(req, timeout) != 0) {
>  		httpc_request_delete(req);
>  		return luaT_error(L);
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index ce9bb9771..3224d8c10 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -296,6 +296,8 @@ end
>  --          'Location' header that a server sends as part of an
>  --          3xx response;
>  --
> +--      accept_encoding - enables automatic decompression of HTTP downloads;

Same here.

Carried the comment to fit 66 symbols limit.

> +--
>  --  Returns:
>  --      {
>  --          status=NUMBER,
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 0/3] httpc: add curl accept_encoding option and relevant fixes

[Alexander Turenko]

LGTM.

> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4232-curlopt-accept-encoding
> Issues: https://github.com/tarantool/tarantool/issues/4232
>         https://github.com/tarantool/tarantool/issues/4579

>   httpc: fix assertion fail after curl write error

Pushed to master, 2.2, 1.10.

>   httpc: add curl accept_encoding option

Pushed to master (with minor wording fixes, answered to the relevant
email).

>   httpc: handle bad Content-Encoding with curl-7.67.0+

Pushed to master, 2.2, 1.10.

Postponed libcurl submodule update until 7.68.0 will be released:
https://github.com/tarantool/tarantool/issues/4698