gzip filter: make sure the stream ends properly #3025

dio · 2018-04-07T15:43:13Z

gzip filter: make sure the stream ends properly

This patch adds additional call of Z_FINISH to properly ends gzip
stream. This also provides a reset functionality since an ended stream
needs to be re-init'ed.

Risk Level: Low

Testing:

Maintain the corresponding unit and integration tests to be PASSED.
Manual test by following repro steps defined in GZip Filter doesn't work on some clients #2909

Docs Changes:
N/A

Release Notes:
N/A

Fixes #2909

Signed-off-by: Dhi Aurrahman dio@rockybars.com

This patch adds additional call of Z_FINISH to properly ends gzip stream. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-04-08T05:58:14Z

@gsagula this is an attempt to try to cure #2909. Thanks!

gsagula · 2018-04-08T06:08:52Z

@dio I'll take a look at. Thank you for fixing it!

mattklein123

Nice, thanks for fixing. Will defer to @gsagula for review but some quick comments.

mattklein123 · 2018-04-08T18:13:08Z

source/common/compressor/zlib_compressor_impl.cc

+  const int result = deflate(zstream_ptr_.get(), Z_FINISH);
+  updateOutput(output_buffer);
+  if (result == Z_OK || result == Z_BUF_ERROR) {
+    finish(output_buffer);


nit: prefer to do this in a loop vs recursively (even though I know the recursion depth will be small).

You could do something similar to deflateNext():

while (deflateNext(Z_FINISH)) { if (zstream_ptr_->avail_out == 0) { updateOutput(output_buffer); } }

... and then handle the logic inside deflateNext(..):

const int result = deflate(zstream_ptr_.get(), flush_state); switch (flush_state) { case Z_FINISH: if (result != Z_OK && result != Z_BUF_ERROR) { RELEASE_ASSERT(result == Z_STREAM_END); return false; } default: if (result == Z_BUF_ERROR && zstream_ptr_->avail_in == 0) { return false; // This means that zlib needs more input, so stop here. } RELEASE_ASSERT(result == Z_OK); } return true;

Just a suggestion, but it is up to you :)

Thanks. This is nicer. 👍

mattklein123 · 2018-04-08T18:14:37Z

source/common/http/filter/gzip_filter.cc

@@ -139,11 +139,15 @@ FilterDataStatus GzipFilter::encodeData(Buffer::Instance& data, bool end_stream)

  if (end_stream) {
    compressor_.flush(compressed_data_);
+    compressor_.finish(compressed_data_);


I haven't look at this code much at all, but it seems like flush/finish finish can be logically combined? Shouldn't finish implicitly flush before finishing?

gsagula · 2018-04-08T19:51:30Z

@dio My apologies for missing the GH notifications about the issue and, thank you again for this fix! It looks great, just a few comments.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula

One problem that I found when implementing this extension using Z_FINISH in combination with transfer-encoding chunked, is that large payloads cause the stream to fail. The only way that I could make this to work was using Z_SYNC_FLUSH, or buffering the entire stream, compressing it and applying Z_FINISH. I'm not one hundred percent sure why, but I would recommend trying to stream something over 100MB just for the sake. I'll try testing this use-case a bit later if I have a chance. Thanks!

gsagula · 2018-04-08T20:11:15Z

source/common/compressor/zlib_compressor_impl.cc

+    // While Z_OK, Z_BUF_ERROR (covered in above case) and Z_STREAM_END are the acceptable values,
+    // the Z_STREAM_ERROR is only possible if the stream is not initialized properly. Ref:
+    // https://zlib.net/zlib_how.html.
+    RELEASE_ASSERT(result == Z_STREAM_END);


I think it's important to have testing coverage here too.

gsagula · 2018-04-08T20:15:41Z

source/common/compressor/zlib_compressor_impl.cc

@@ -82,5 +82,28 @@ void ZlibCompressorImpl::updateOutput(Buffer::Instance& output_buffer) {
  zstream_ptr_->next_out = chunk_char_ptr_.get();
 }

+void ZlibCompressorImpl::reset() {
+  const bool result = deflateReset(zstream_ptr_.get());
+  RELEASE_ASSERT(result == Z_OK);


I think we need some death tests to exercise this logic.

OK. Will try to have it.

gsagula · 2018-04-08T21:12:45Z

source/common/compressor/zlib_compressor_impl.cc

+  const int result = deflate(zstream_ptr_.get(), Z_FINISH);
+  updateOutput(output_buffer);
+  if (result == Z_OK || result == Z_BUF_ERROR) {
+    finish(output_buffer);


You could do something similar to deflateNext():

while (deflateNext(Z_FINISH)) { if (zstream_ptr_->avail_out == 0) { updateOutput(output_buffer); } }

... and then handle the logic inside deflateNext(..):

const int result = deflate(zstream_ptr_.get(), flush_state); switch (flush_state) { case Z_FINISH: if (result != Z_OK && result != Z_BUF_ERROR) { RELEASE_ASSERT(result == Z_STREAM_END); return false; } default: if (result == Z_BUF_ERROR && zstream_ptr_->avail_in == 0) { return false; // This means that zlib needs more input, so stop here. } RELEASE_ASSERT(result == Z_OK); } return true;

Just a suggestion, but it is up to you :)

gsagula · 2018-04-08T21:35:01Z

source/common/compressor/zlib_compressor_impl.h

@@ -81,6 +81,18 @@ class ZlibCompressorImpl : public Compressor {
   */
  uint64_t checksum();

+  /**
+   * This is required to re-init the stream after calling finish(_).


I think deflateReset() does not deallocate memory. It just reset the counters and returns the stream results. Can you add a line to the comment explaining this?
From zlib manual:

This function is equivalent to deflateEnd followed by deflateInit, but does not free and reallocate the internal compression state. The stream will leave the compression level and any other attributes that may have been set unchanged.

dio · 2018-04-09T01:00:12Z

@gsagula yes, that came into my mind while proposing this PR. Will try to have a test on it.

Confirmed to be working with streaming file > 100MB. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-04-09T07:34:34Z

@gsagula, so I have this simple test server in node (sorry for some .js 😆) serving a small .txt file and an over 100MB .pdf file. The following gives you Transfer-Encoding: chunked,

const Server = require("http").Server;
const fs = require("fs");
const server = new Server((req, res) => {
  return fs
    .createReadStream(req.url == "/book" ? "book.pdf" : "a.txt")
    .pipe(res);
});

server.listen(1234);

Then,

Run and put the server as an upstream host
Get the book.pdf using curl localhost:9000/book -H 'Host: test-server.com' -H 'Accept-Encoding: gzip' -o book1.pdf.gz
gunzip the dumped file.
Open the file.

Seems fine.

gsagula · 2018-04-09T16:49:22Z

@dio That's awesome! I will do a final pass and some more manual testing.
Thank you for all the work 👍

ps. (Js and Crypto) are the two words that make things popular these days :)

gsagula · 2018-04-10T08:24:13Z

test/common/compressor/zlib_compressor_impl_test.cc

@@ -41,7 +84,14 @@ class ZlibCompressorImplDeathTest : public ZlibCompressorImplTest {
 TEST_F(ZlibCompressorImplDeathTest, CompressorTestDeath) {
  EXPECT_DEATH(compressorBadInitTestHelper(100, 8), std::string{"assert failure: result >= 0"});
  EXPECT_DEATH(compressorBadInitTestHelper(31, 10), std::string{"assert failure: result >= 0"});
-  EXPECT_DEATH(unitializedCompressorTestHelper(), std::string{"assert failure: result == Z_OK"});
+  EXPECT_DEATH(uninitializedCompressorTestHelper(), std::string{"assert failure: result == Z_OK"});


Thanks for adding more death tests.

gsagula · 2018-04-10T08:24:57Z

test/common/compressor/zlib_compressor_impl_test.cc

-                  compressed_slices[num_comp_slices - 1].len_);
-  // FOOTER four-byte sequence (sync flush)
-  EXPECT_EQ("0000ffff", footer_hex_str.substr(footer_hex_str.size() - 8, 10));
+  expectValidCompressedBuffer(output_buffer);


This looks much cleaner!

gsagula

LGTM, I will defer to @dnoe if he also wants to take a quick look. Thanks for the fix.

mattklein123 · 2018-04-10T17:53:31Z

source/common/compressor/zlib_compressor_impl.cc

@@ -68,18 +68,41 @@ void ZlibCompressorImpl::process(Buffer::Instance& output_buffer, int64_t flush_
  }

  if (flush_state == Z_SYNC_FLUSH) {


nit/question: IMO it's a bit strange to finish in the context of a sync_flush operation. They are distinct operations from the deflate perspective. Since I think now when we call flush() we actually want to finish(), should we remove the flush() method for now and just add a direct finish() method?

@mattklein123 yes, from my test it works fine to just calling finish() from the filter.

Since the flush() would not be used anymore, I think I need to: update the tests as well. Since the current tests are around Z_SYNC_FLUSH.

Or should we just keep the flush() method in ZlibCompressorImp?

I would prefer we delete methods we don't actually need. We can always add them back later. @gsagula WDYT?

@mattklein123 I'm in favor of deleting any method that is not being used. @dio It makes sense to replace flush() with finish(), but we still use Z_SYNC_FLUSH internally, correct? FYI, as soon as I get a chance I would like to revisit this entire implementation anyway, so don't worry too much about the design.

It makes sense to replace flush() with finish(), but we still use Z_SYNC_FLUSH internally, correct?

I think without Z_SYNC_FLUSH-ing it is fine. Z_SYNC_FLUSH adds that 0000ffff right before crc32 and isize, which probably it is not necessary?

Will push a commit and let you know.

FYI, as soon as I get a chance I would like to revisit this entire implementation anyway, so don't worry too much about the design.

Fantastic! Probably we can include deflate and deflateRaw as well 🙂

I think without Z_SYNC_FLUSH-ing it is fine. Z_SYNC_FLUSH adds that 0000ffff right before crc32 and isize, which probably it is not necessary?

@dio Good point, but I think we still need sync for byte padding (I need to look into it). Here is what others have done. It seems that Z_SYNC_FLUSH is still broadly used.

https://github.com/apache/trafficserver/blob/711c4f4be103b44c1971196297c373d7684a0db5/plugins/gzip/gzip.cc#L319

https://github.com/facebook/proxygen/blob/1084545a0015b65882e3256fe18f505942472d40/proxygen/lib/utils/ZlibStreamCompressor.cpp#L146

https://github.com/nginx/nginx/blob/53eae18826d13cb4c7db6680029cac53bfb2a18a/src/http/modules/ngx_http_gzip_filter_module.c#L745

Fantastic! Probably we can include deflate and deflateRaw as well

I think to do one for Brotli as well would be cool. Some interesting use-cases can be explored when multiple compression types are available :)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula

@dio Did you see my comment about Z_SYNC_FLUSH? I'm not sure if we can remove it. I'll need to look into it.

dio · 2018-04-12T00:25:22Z

@gsagula sorry for missing that. Will revert back and investigate.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula · 2018-04-17T16:43:34Z

source/common/compressor/zlib_compressor_impl.cc

@@ -42,10 +42,10 @@ void ZlibCompressorImpl::compress(Buffer::Instance& buffer, bool trailer) {
    zstream_ptr_->avail_in = input_slice.len_;
    zstream_ptr_->next_in = static_cast<Bytef*>(input_slice.mem_);
    process(buffer, Z_NO_FLUSH);
+    buffer.drain(input_slice.len_);


I see that you found a way to use no_flush. Did you check if it works with multiple slices?

I have a test in here a291b5c.

gsagula

Thanks @dio! Just couple comments. I would also make sure that the following use cases work:

stream a large text file and save it in .gz format (the actual bug)
stream a large text to clients (browser/curl --compressed)
stream a non-compressible file (it actually expands)

gsagula · 2018-04-17T16:58:12Z

source/common/compressor/zlib_compressor_impl.cc

+      RELEASE_ASSERT(result == Z_STREAM_END);
+      return false;
+    }
+  default:


Would this logic work with any other zlib flush state, e.g. Z_PARTIAL_FLUSH?

Will definitely check this out.

@gsagula the Z_PARTIAL_FLUSH works confirmed by a quick manual test for a small and big file stream from upstream. (the unit tests obviously need to be adapted, do you want me to make it an option i.e. either to use SYNC_FLUSH or PARTIAL_FLUSH?).

No, I was just wondering if other states also work since the currently logic will default to any that is not Z_FINISH. Thanks for testing it.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-04-18T07:00:54Z

@gsagula

stream a large text file and save it in .gz format (the actual bug)

I have manually tested it. And I think it is passed.

stream a large text to clients (browser/curl --compressed)

I have manually tested it. And I think it is passed.

stream a non-compressible file (it actually expands)

I can see that currently, the filter by default compresses the stream (if the accept-encoding is required and transferred as chunks). I think we should by default don't compress and only care for whitelisted content-type.

I can do detection (of the magic numbers) but it seems too much and will introduce false-positive?

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula · 2018-04-18T16:05:44Z

I can see that currently, the filter by default compresses the stream (if the accept-encoding is required and transferred as chunks). I think we should by default don't compress and only care for whitelisted content-type.

I'm not sure I follow this one. Would mind to elaborate it, please?

I can do detection (of the magic numbers) but it seems too much and will introduce false-positive?

I think checking for the magic number is still not reliable. One common idiom is to buffer the entire response, compress it and, verify that didn't expand. However, this approach would have other side-effects such as latency and memory. For now, we just compress anything and assume that most of the time this filter will be receiving compressible content.

dio · 2018-04-18T22:27:41Z

I'm not sure I follow this one. Would mind to elaborate it, please?

@gsagula e.g. having this:

          http_filters:
          - name: envoy.gzip
            config:
              memory_level: 9
              compression_level: BEST

when a client asks for Accept-Encoding: gzip, the filter compresses the upstream's stream regardless Content-Type (hence even when the chunks from the upstream are already compressed, the filter compresses again)

gsagula · 2018-04-18T22:54:14Z

the filter compresses the upstream's stream regardless Content-Type

I don't think so, it validates against the default ist if none is specified in the config:

A response does not contain a content-type value that matches one of the selected mime-types, which default to application/javascript, application/json, application/xhtml+xml, image/svg+xml, text/css, text/html, text/plain, text/xml.

https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/gzip_filter.html?highlight=gzip#how-it-works

envoy/source/extensions/filters/http/gzip/gzip_filter.cc

Line 32 in 55a95f4

CONSTRUCT_ON_FIRST_USE(std::vector<std::string>,

dio · 2018-04-18T23:11:46Z

@gsagula Hum, it seems my test server doesn't send Content-Type. Haha. So it is being hit by

https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/gzip/gzip_filter.cc#L205

Sorry for false alarm.

So yeah, I think I have your concerns tested now.

gsagula · 2018-04-19T04:46:31Z

test/common/decompressor/zlib_decompressor_impl_test.cc

+    ASSERT_EQ(0, buffer.length());
+
+    compressor.compress(buffer, Compressor::State::Finish);
+    ASSERT_GE(10, buffer.length());


That's a nice helper! Can you add a small comment explaining where this 10 is coming from?

Think this one is part of my debugging artifacts. Guess it should be 6, but I will remove it for now to avoid confusion.

gsagula · 2018-04-19T04:56:21Z

test/common/decompressor/zlib_decompressor_impl_test.cc

+        sample.c_str(), sample.size(),
+        [](const void*, size_t, const Buffer::BufferFragmentImpl* frag) { delete frag; });
+
+    buffer.addBufferFragment(*frag);


I think we should use this for all the other tests too, but no worries for now. We can do that in a separated PR when revisiting this implementation.

gsagula · 2018-04-19T04:58:21Z

test/test_common/utility.h

+   * @param bytes input bytes.
+   * @return Type flip-ordered bytes.
+   */
+  template <class Type> static Type flipOrder(const Type& bytes) {


Where is this being used?

https://github.com/dio/envoy/blob/3f3a1757b08128dcd7f21c9a6980c5d4c9f33e69/test/common/compressor/zlib_compressor_impl_test.cc#L60 to check the correctness of stored ISIZE after a buffer is Z_FINISH-ed

Oh sorry, I didn't see.

gsagula · 2018-04-19T05:12:28Z

LGTM, Thanks for correcting it. Just one minor comment.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

mattklein123

LGTM, just a small question/comment and also needs a master merge.

mattklein123 · 2018-04-19T17:25:19Z

include/envoy/compressor/compressor.h

-   * @param input_buffer supplies the buffer with data to be compressed.
-   * @param output_buffer supplies the buffer to output compressed data.
+   * Compresses data buffer.
+   * @param buffer supplies the reference to data to be compressed.


Can you specify that the buffer contents will be replaced inline with compressed data?

mattklein123 · 2018-04-19T17:32:53Z

source/common/compressor/zlib_compressor_impl.cc


  for (const Buffer::RawSlice& input_slice : slices) {
    zstream_ptr_->avail_in = input_slice.len_;
    zstream_ptr_->next_in = static_cast<Bytef*>(input_slice.mem_);
-    process(output_buffer, Z_NO_FLUSH);
+    process(buffer, Z_NO_FLUSH);


Can you add an ASSERT into process that confirms that in the case of Z_NO_FLUSH no data is added to the buffer? Is that always guaranteed? I'm still a little concerned about the in/out buffer behavior here. Otherwise can you add more comments?

It turns out if with RLE, SPEED and 1 as memory level, I can see a case when data is being added to the buffer when flush_state == Z_NO_FLUSH. Will try to debug it.

Hi @mattklein123 You probably saw something that we didn't see. Question, even if data is added to the buffer, what the consequences would be?

I think it will work, because you add data to the end, and you drain from the beginning. Also, you snap the slices before you start adding, so you should never drain compressed data. However, it's not super intuitive how this works, so if you keep it this way I would add a bunch of comments (I think it's probably fine w/ comments).

I agree that this is not the most intuitive design. I'm planning on revisiting this implementation as soon as I get some stuff out of my way. What @dio and I agreed, was to go for correctness first. We will add the comments though. Thanks for the input.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-04-20T00:42:48Z

Thanks, @mattklein123! I have added the comments and done a master merge.

mattklein123

Thank you @dio and @gsagula! Awesome work!

We suspect that the gzip filter is under performing due to excessive flushes. The changes introduced in envoyproxy#3025 -- which were an attempt to fix envoyproxy#2909 -- are causing too many flushes (one per encodeData() call) to ensure the compression ise properly finished. I think at that point it was possible for encodeData() to be call with end_stream=false while encodeTrailers() wasn't handle so Z_FINISH could be missed. This isn't the case anymore, so there's no need to flush every time we encode a chunk of th response. We could make this configurable, but given it's a considerable perf regression _and_ that we'd still be doing the correct thing by just calling Z_FINISH when end_stream=true or from encodeTrailers(), I don't think it's worth it. This also adds stats for visibility. Helps with envoyproxy#8448. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Fix improper gzip end stream

d3c3ded

This patch adds additional call of Z_FINISH to properly ends gzip stream. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio mentioned this pull request Apr 8, 2018

GZip Filter doesn't work on some clients #2909

Closed

mattklein123 reviewed Apr 8, 2018

View reviewed changes

Review comments: remove finish() and prefer loop

86e4b86

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula suggested changes Apr 9, 2018

View reviewed changes

dio added 5 commits April 9, 2018 11:29

Move reset

9a0d529

Confirmed to be working with streaming file > 100MB. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix tests

d0c4239

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add death tests

74063b9

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove new lines

d8239b3

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix typo

56b5e88

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dnoe assigned mattklein123 and gsagula Apr 9, 2018

gsagula reviewed Apr 10, 2018

View reviewed changes

gsagula approved these changes Apr 10, 2018

View reviewed changes

mattklein123 reviewed Apr 10, 2018

View reviewed changes

dio added 6 commits April 11, 2018 10:53

Remove flush()

6552468

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove require_finish flag

767fb60

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add flipOrder helper

2b568b9

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix format

dff37df

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

9b010f0

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Use finish()

d44d929

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula suggested changes Apr 11, 2018

View reviewed changes

dio added 4 commits April 17, 2018 14:24

Implement the new compressor interface

467df23

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Update tests

467af66

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

d8f923e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add test of compress-decompress with different memory levels

ebd3e6d

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula reviewed Apr 17, 2018

View reviewed changes

Add test with multiple slices of buffer

a291b5c

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio added 2 commits April 18, 2018 14:42

Dynamically allocate the fragments

4a1dd67

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Lambda capture 'this' is not used

3f3a175

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula reviewed Apr 19, 2018

View reviewed changes

Remove debugging assertion

9ef74ad

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

gsagula approved these changes Apr 19, 2018

View reviewed changes

mattklein123 reviewed Apr 19, 2018

View reviewed changes

dio added 4 commits April 20, 2018 06:52

Merge remote-tracking branch 'upstream/master'

0523291

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add comments

e10d6aa

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove superfluous string constructions

b36f2fc

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Comment revision

ac6ec76

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

mattklein123 approved these changes Apr 20, 2018

View reviewed changes

mattklein123 merged commit 9c3b1b2 into envoyproxy:master Apr 20, 2018

alexandrecp-b mentioned this pull request May 10, 2019

GZip filter + grpc_json_transcoder doesn't work on some client #6894

Closed

rgs1 mentioned this pull request Mar 24, 2020

envoy.gzip compression performance is poor #8448

Open

rgs1 mentioned this pull request Mar 25, 2020

compressor: don't flush on every encodeData() call #10518

Closed

		@@ -68,18 +68,41 @@ void ZlibCompressorImpl::process(Buffer::Instance& output_buffer, int64_t flush_
		}

		if (flush_state == Z_SYNC_FLUSH) {

gzip filter: make sure the stream ends properly #3025

gzip filter: make sure the stream ends properly #3025

Conversation

dio commented Apr 7, 2018 • edited Loading

dio commented Apr 8, 2018

gsagula commented Apr 8, 2018 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula commented Apr 8, 2018

gsagula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 9, 2018 • edited Loading

dio commented Apr 9, 2018 • edited Loading

gsagula commented Apr 9, 2018 • edited Loading

gsagula Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

gsagula Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

gsagula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

dio Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

gsagula Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

gsagula left a comment

Choose a reason for hiding this comment

dio commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

dio Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 18, 2018 • edited Loading

gsagula commented Apr 18, 2018 • edited Loading

dio commented Apr 18, 2018

gsagula commented Apr 18, 2018 • edited Loading

dio commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula commented Apr 19, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

dio commented Apr 20, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

dio commented Apr 7, 2018 •

edited

Loading

gsagula commented Apr 8, 2018 •

edited

Loading

dio commented Apr 9, 2018 •

edited

Loading

dio commented Apr 9, 2018 •

edited

Loading

gsagula commented Apr 9, 2018 •

edited

Loading

gsagula Apr 10, 2018 •

edited

Loading

gsagula Apr 10, 2018 •

edited

Loading

dio Apr 10, 2018 •

edited

Loading

gsagula Apr 10, 2018 •

edited

Loading

dio Apr 11, 2018 •

edited

Loading

gsagula Apr 11, 2018 •

edited

Loading

dio Apr 18, 2018 •

edited

Loading

dio Apr 18, 2018 •

edited

Loading

dio commented Apr 18, 2018 •

edited

Loading

gsagula commented Apr 18, 2018 •

edited

Loading

gsagula commented Apr 18, 2018 •

edited

Loading

dio Apr 19, 2018 •

edited

Loading

gsagula Apr 19, 2018 •

edited

Loading