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

Set compression to curl defaults #495

Merged
merged 1 commit into from
Feb 3, 2018
Merged

Set compression to curl defaults #495

merged 1 commit into from
Feb 3, 2018

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Jan 30, 2018

While zlib is mandatory for transmission, it is not mandatory for curl.

A libcurl that has been compiled with no support for zlib will return no data if compressed responses are set to on.

In the basic case this prevents the port checking functionality from working properly. It also prevents web seeding from working as well.

@@ -183,7 +183,6 @@ static CURL* createEasy(tr_session* s, struct tr_web* web, struct tr_web_task* t
task->timeout_secs = getTimeoutFromURL(task);

curl_easy_setopt(e, CURLOPT_AUTOREFERER, 1L);
curl_easy_setopt(e, CURLOPT_ENCODING, "gzip;q=1.0, deflate, identity");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of unconditionally refusing to accept compressed responses, I'd rather check whether curl_version_info() indicates support for CURL_VERSION_LIBZ feature. Obsiously, not on each and every request :)

@cfpp2p
Copy link

cfpp2p commented Jan 31, 2018

@neheb @mikedld

tells curl to fill in the blanks with what it was compiled to support

ref: https://trac.transmissionbt.com/changeset/8658

curl_easy_setopt(e, CURLOPT_ENCODING, "");

@neheb
Copy link
Contributor Author

neheb commented Jan 31, 2018

Isn't curl_easy_setopt(e, CURLOPT_ENCODING, ""); a default setting?

@cfpp2p
Copy link

cfpp2p commented Jan 31, 2018

Isn't curl_easy_setopt(e, CURLOPT_ENCODING, ""); a default setting?

@neheb
Documentation seems ambiguous
curl/curl#785

Looking at source for curl-7.58.0

configure defines HAVE_LIBZ

content_encoding.c


/* supported content encodings table. */
static const content_encoding * const encodings[] = {
  &identity_encoding,
#ifdef HAVE_LIBZ
  &deflate_encoding,
  &gzip_encoding,
#endif
#ifdef HAVE_BROTLI
  &brotli_encoding,
#endif
  NULL
};

static const content_encoding deflate_encoding = {
  "deflate",
  NULL,
  deflate_init_writer,
  deflate_unencode_write,
  deflate_close_writer,
  sizeof(zlib_params)
};

static const content_encoding gzip_encoding = {
  "gzip",
  "x-gzip",
  gzip_init_writer,
  gzip_unencode_write,
  gzip_close_writer,
  sizeof(zlib_params)
};

@neheb
Copy link
Contributor Author

neheb commented Jan 31, 2018

So the default is whatever is supported by curl. What's the point in manually specifying it then. Or even doing checks?

@V-E-O
Copy link

V-E-O commented Feb 1, 2018

@neheb According to latest doc from libcurl https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html, in default the encoding is NULL which means not sending an Accept-Encoding header. In most cases, it works well except for some weird servers arbitrarily sending compressed data if no Accept-Encoding.

While zlib is mandatory for transmission, it is not mandatory for curl.

A libcurl that has been compiled with no support for zlib will return no data if compressed responses are set to on.

In the basic case this prevents the port checking functionality from working properly. It also prevents web seeding from working as well.
@neheb neheb changed the title Remove compression from Curl options Set compression to curl defaults Feb 1, 2018
@neheb
Copy link
Contributor Author

neheb commented Feb 1, 2018

Nice and clean. I like it. Force pushed a new commit.

This has the side benefit of also supporting brotli compression if curl is compiled with it.

@neheb
Copy link
Contributor Author

neheb commented Feb 2, 2018

@mikedld any complaints? I feel that there's no point to doing LIBZ checks since curl does it anyway.

@cfpp2p
Copy link

cfpp2p commented Feb 2, 2018

@neheb
NULL is not the same as ""
What you want is:

curl_easy_setopt(e, CURLOPT_ENCODING, "");

Tested each way. The results are as follows

curl_easy_setopt(e, CURLOPT_ENCODING, "");

GET /51414 HTTP/1.1
User-Agent: Transmission/2.93
Host: portcheck.transmissionbt.com
Accept: */*
Accept-Encoding: deflate, gzip, br


NULL (did NOT set CURLOPT_ENCODING at ALL!)

GET /51414 HTTP/1.1
User-Agent: Transmission/2.93
Host: portcheck.transmissionbt.com
Accept: */*


curl_easy_setopt(e, CURLOPT_ENCODING, "gzip;q=1.0, deflate, identity");

GET /51414 HTTP/1.1
User-Agent: Transmission/2.93
Host: portcheck.transmissionbt.com
Accept: */*
Accept-Encoding: gzip;q=1.0, deflate, identity

@neheb
Copy link
Contributor Author

neheb commented Feb 2, 2018

that's exactly what i force pushed...

@cfpp2p
Copy link

cfpp2p commented Feb 2, 2018

OK, sorry, looks good. I didn't see that.

cfpp2p pushed a commit to cfpp2p/transmission that referenced this pull request Feb 2, 2018
cfpp2p pushed a commit to cfpp2p/transmission that referenced this pull request Feb 2, 2018
@cfpp2p
Copy link

cfpp2p commented Feb 3, 2018

both transmission's remote.c and show.c already are using

curl_easy_setopt(curl, CURLOPT_ENCODING, "");
/* "" tells curl to fill in the blanks with what it was compiled to support */

Copy link
Member

@mikedld mikedld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. I didn't dive into the documentation deep enough when I suggested to add a feature check ;)

@mikedld
Copy link
Member

mikedld commented Feb 3, 2018

Thanks for the input @cfpp2p, as always!

@mikedld mikedld merged commit 2c15ca0 into transmission:master Feb 3, 2018
@mikedld mikedld added this to the 3.00 milestone Feb 3, 2018
@neheb neheb deleted the patch-1 branch February 3, 2018 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants