Ticket21377 035 01 WIP

[juga0]

This branch is not intended to be merged, but to debug why the bandwidth file content is not being compressed.
appveyor fails in a different test, but i don't think CI messages are useful for this.

What has being useful for me is to run src/test/test --debug dir_handle_get/status_vote_next_bandwidth. This is the relevant output:

ec 20 11:39:04.229 [debug] directory_handle_command_get__real(): Received GET command.
Dec 20 11:39:04.229 [debug] directory_handle_command_get__real(): rewritten url as '"/tor/status-vote/next/bandwidth.z"'.
Dec 20 11:39:04.229 [debug] directory_handle_command_get__real(): compression supported 5
Dec 20 11:39:04.229 [debug] handle_get_next_bandwidth(): Getting next bandwidth.
Dec 20 11:39:04.229 [debug] handle_get_next_bandwidth(): compress_method 2
Dec 20 11:39:04.230 [debug] handle_get_next_bandwidth(): Calling connection_buf_add_compress.
Dec 20 11:39:04.230 [debug] fetch_from_buf_http(): headerlen 149, bodylen 167.
Dec 20 11:39:04.230 [debug] test_dir_handle_get_status_vote_next_bandwidth(): HTTP/1.0 200 OK
Date: Thu, 20 Dec 2018 11:39:04 GMT
Content-Type: text/plain
Content-Encoding: deflate
Expires: Thu, 20 Dec 2018 12:09:04 GMT

[teor2345]

In rare cases, compression can result in data that is the same size, or even slightly larger.
If you want to test if the content is compressed, test that it is not the same as the uncompressed content.

[teor2345]

This code sets the compression state on the connection, then adds the data uncompressed.

Here's some similar code that correctly adds the data compressed:

tor/src/feature/dircache/dircache.c

Lines 1034 to 1048 in 8020d6f

	write_http_response_header(conn, body_len ? body_len : -1,
	compress_method,
	lifetime);
	if (smartlist_len(items)) {
	if (compress_method != NO_METHOD) {
	conn->compress_state = tor_compress_new(1, compress_method,
	choose_compression_level(estimated_len));
	SMARTLIST_FOREACH(items, const char *, c,
	connection_buf_add_compress(c, strlen(c), conn, 0));
	connection_buf_add_compress("", 0, conn, 1);
	} else {
	SMARTLIST_FOREACH(items, const char *, c,
	connection_buf_add(c, strlen(c), TO_CONN(conn)));
	}

tor/src/feature/dircache/dircache.c

Lines 1292 to 1310 in 8020d6f

	write_http_response_header(conn,
	compress_method != NO_METHOD ? -1 : len,
	compress_method,
	60*60);
	if (compress_method != NO_METHOD) {
	conn->compress_state = tor_compress_new(1, compress_method,
	choose_compression_level(len));
	SMARTLIST_FOREACH(certs, authority_cert_t *, c,
	connection_buf_add_compress(
	c->cache_info.signed_descriptor_body,
	c->cache_info.signed_descriptor_len,
	conn, 0));
	connection_buf_add_compress("", 0, conn, 1);
	} else {
	SMARTLIST_FOREACH(certs, authority_cert_t *, c,
	connection_buf_add(c->cache_info.signed_descriptor_body,
	c->cache_info.signed_descriptor_len,
	TO_CONN(conn)));
	}

It's not great to copy code, but I opened a ticket to clean up:
https://trac.torproject.org/projects/tor/ticket/28815

[juga0]

Ah, i see, connection_buf_add_compress, not connection_buf_add

[juga0]

Now, using connection_buf_add_compress, but detect_compression_method still returns NO_METHOD, and

tt_str_op(content, OP_NE, comp_body);

still fails.
So, it seems that even if find_best_compression_method returns ZLIB and the header has Content-Encoding: deflate, the data is not compressed and it's the same.
I'm not sure if i'd need to try with a bigger string.

[juga0]

Also thanks for pointing out the code copied, i wondered about it.

[nmathewson]

So one easy way to debug this would be to fetch and log the entire contents of "TO_CONN(conn)->outbuf" right before the fetch_from_buf_http. That way you'll be able to see whether it's compressing or not, and localize the failure there.

Incidentally, strlen(content)+1 is not a great upper bound for the compressed body length, since it could be larger than the original content.

Also, the compressed body, if it's really compressed, is going to be a sequence of bytes with some 0 bytes, so you can't rely on it being nul-terminated, so you can't use strcmp() or tt_str_op() on it.

[nmathewson]

If it turns out that the data isnt' being compressed, one way to debug that would be to log the chosen compression_method from inside directory_handle_command_get.

[juga0]

logging that says actually that the compression_method is 5 (UNKNOWN_METHOD).
Why is happening this?, because the body is too short? (167 bytes).

[teor2345]

Now, using connection_buf_add_compress, but detect_compression_method still returns NO_METHOD

I can't find this new code.
Please open a pull request.

[coveralls]

Pull Request Test Coverage Report for Build 3357

• 18 of 20 (90.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 61.012%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/dircache/dircache.c	18	20	90.0%

Totals
Change from base Build 3356:	0.01%
Covered Lines:	43857
Relevant Lines:	71882

---

💛 - Coveralls

[nmathewson]

Does the problem happen outside the tests?

If not, here's what I think is happening. I think the reason things aren't getting compressed is that connection_write_to_buf_impl_ is mocked in that test, and connection_write_to_buf_mock does not actually do any compression.

[teor2345]

21377 was merged.