Repository: apple/swift-nio-extras (swift)

Issue:
HTTPResponseCompressor creates responses that gzip cannot decompress
HTTPResponseCompressor creates response bodies that cannot be decompressed by command line `gzip`, as well as other tools that use a zlib-based gzip stream decompressor, such as nodejs.

I've found this issue originally because VSCode is in the process of switching to a new javascript debugger, which broke loading of source maps for my [Vapor](https://github.com/vapor/vapor) app. It turns out that the reason for this is that they switched to a (very popular) Javascript library called [got](https://github.com/sindresorhus/got) to load source maps, which pipes compressed responses through a zlib-based stream decoder, and that fails to decompress the responses generated by HTTPResponseCompressor.

The problem can also be reproduced on the command line (see below).

I've originally opened the issue with Vapor [here](https://github.com/vapor/vapor/issues/2449), which has some additional info as well.

For reference, my original issue with vscode-js-debug (which triggered the issue) is [here](https://github.com/microsoft/vscode-js-debug/issues/650).

Browsers, as well as some other http-related tools, seem to be more lenient with the responses and decompress them just fine, but gzip and the zlib-bindings of nodejs do not (and maybe other zlib-based tools as well).

### Expected behavior

Piping a gzip-compressed response through command-line `gzip` should decompress it just fine. Here is an example from `google.com` (though it can be tried with any other server able to produce a gzipped response):

```
xxx@yyy$ curl -H 'Accept-encoding: gzip' --output /tmp/other-compressed-response.gz 'http://www.google.com'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5186  100  5186    0     0  50349      0 --:--:-- --:--:-- --:--:-- 50349

xxx@yyy$ file /tmp/other-compressed-response.gz 
/tmp/other-compressed-response.gz: gzip compressed data, max compression, original size modulo 2^32 11804

xxx@yyy$ gunzip -tv /tmp/other-compressed-response.gz 
/tmp/other-compressed-response.gz:	  OK
```

E.g. `gzip` should decompress the response fine, and the `file` utility should show the correct decompressed length of the stream.

### Actual behavior
However, using a simple Vapor demo app (which directly uses HTTPResponseCompresssor) yields the following results.


```
xxx@yyy$ curl -H 'Accept-encoding: gzip' --output /tmp/swift-nio-compressed-response.gz 'http://127.0.0.1:8080/index.html'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   247    0   247    0     0  61750      0 --:--:-- --:--:-- --:--:-- 61750

xxx@yyy$ file /tmp/swift-nio-compressed-response.gz
/tmp/swift-nio-compressed-response.gz: gzip compressed data, original size modulo 2^32 4294901760 gzip compressed data, reserved method, ASCII, has CRC, extra field, has comment, encrypted, from FAT filesystem (MS-DOS, OS/2, NT), original size modulo 2^32 4294901760

xxx@yyy$ gunzip -tv /tmp/swift-nio-compressed-response.gz
gunzip: /tmp/swift-nio-compressed-response.gz: unexpected end of file
gunzip: /tmp/swift-nio-compressed-response.gz: uncompress failed
/tmp/swift-nio-compressed-response.gz:	  NOT OK
```

Using this demo app, `gzip` fails to decompress the gzip response. Also, the `file` tool shows a really weird header, e.g. decompressed size shows up as `4294901760` (that's the actual issue – the failing tools all claim that the stream ends prematurely), and lots of extra header flags that appear to be unintentional.

The minimal Vapor demo app can be dowloaded in the issue at vapor [here](https://github.com/vapor/vapor/issues/2449#issuecomment-663228266) -> Vapor uses the response decompressor directly (https://github.com/vapor/vapor/blob/master/Sources/Vapor/HTTP/Server/HTTPServer.swift#L396-L405), so it's likely the problem is with NIO, rather than Vapor (though this is an assumption).

### Steps to reproduce

- Download [demo.zip](https://github.com/vapor/vapor/files/4968711/demo.zip) from the issue at Vapor.
- Build it
- Request the index.html file via cURL to the filesystem, but do not add the `--decompress` flag (cURL itself *can* decompress it too, like all browsers)
- Check the compressed response with `file` or try to decompress with `gzip`

- Alternatively (for the last step), you can also try it with nodejs's zlib bindings, like used in "got" -> I've added sample code for this in [Vapor issue](https://github.com/vapor/vapor/issues/2449#issue-664616274) in the top-most post.

### If possible, minimal yet complete reproducer code (or URL to code)

[demo.zip](https://github.com/vapor/vapor/files/4968711/demo.zip) – It's a Vapor app, though. I'd have to look into how to make a more minimal version, using just swift-nio-extras.

### SwiftNIO-Extras version/commit hash

```
"revision": "7cd24c0efcf9700033f671b6a8eaa64a77dd0b72",
"version": "1.5.1"
```

### Swift & OS version (output of `swift --version && uname -a`)

```
Apple Swift version 5.2.4 (swiftlang-1103.0.32.9 clang-1103.0.32.53)
Target: x86_64-apple-darwin19.4.0
Darwin <redacted> 19.4.0 Darwin Kernel Version 19.4.0: Wed Mar  4 22:28:40 PST 2020; root:xnu-6153.101.6~15/RELEASE_X86_64 x86_64
```

Pull request:
Fixes #99.

Please check if the test makes sense like this – It would also be possible to implement this write order as a `WriteStrategy` for `compressResponse()` and detect the erroneous handling in `decompress()` later.

### Motivation:

Previously, when using the response compressor, doing a flush() right before finishing the
response data would cause the final compression chunk to be omitted. Some strict decompressors
(such as gzip or the zlib functionality exported in nodejs) would refuse to decompress the
incomplete response.

With this change, the generated compressed response is properly finalized.

### Modifications:

In HTTPResponseCompressor.swift, a channel write is now also generated if no body data is
added, but a flush is required.

### Result:

The response is now correct for this edge case, enabling gzip, nodejs, et al., to
decompress it without errors.

Patch:
diff --git a/Sources/NIOHTTPCompression/HTTPResponseCompressor.swift b/Sources/NIOHTTPCompression/HTTPResponseCompressor.swift
index 108b1c28..ef3fdc66 100644
--- a/Sources/NIOHTTPCompression/HTTPResponseCompressor.swift
+++ b/Sources/NIOHTTPCompression/HTTPResponseCompressor.swift
@@ -280,7 +280,7 @@ private struct PartialHTTPResponse {
     /// buffer and losing all copies of the other HTTP data. At this point it may freely be reused.
     mutating func flush(compressor: inout NIOCompression.Compressor, allocator: ByteBufferAllocator) -> (HTTPResponseHead?, ByteBuffer?, HTTPServerResponsePart?) {
         var outputBody: ByteBuffer? = nil
-        if self.body.readableBytes > 0 {
+        if self.body.readableBytes > 0 || mustFlush {
             let compressedBody = compressor.compress(inputBuffer: &self.body, allocator: allocator, finalise: mustFlush)
             if isCompleteResponse {
                 head!.headers.remove(name: "transfer-encoding")


Test patch:
diff --git a/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest+XCTest.swift b/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest+XCTest.swift
index 3159ca30..1be67914 100644
--- a/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest+XCTest.swift
+++ b/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest+XCTest.swift
@@ -44,6 +44,7 @@ extension HTTPResponseCompressorTest {
                 ("testOverridesContentEncodingHeader", testOverridesContentEncodingHeader),
                 ("testRemovingHandlerFailsPendingWrites", testRemovingHandlerFailsPendingWrites),
                 ("testDoesNotBufferWritesNoAlgorithm", testDoesNotBufferWritesNoAlgorithm),
+                ("testChunkedGzipResponseProducesCorrectNumberOfWrites", testChunkedGzipResponseProducesCorrectNumberOfWrites),
                 ("testStartsWithSameUnicodeScalarsWorksOnEmptyStrings", testStartsWithSameUnicodeScalarsWorksOnEmptyStrings),
                 ("testStartsWithSameUnicodeScalarsWorksOnLongerNeedleFalse", testStartsWithSameUnicodeScalarsWorksOnLongerNeedleFalse),
                 ("testStartsWithSameUnicodeScalarsWorksOnSameStrings", testStartsWithSameUnicodeScalarsWorksOnSameStrings),
diff --git a/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest.swift b/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest.swift
index 6824b622..4d3651ab 100644
--- a/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest.swift
+++ b/Tests/NIOHTTPCompressionTests/HTTPResponseCompressorTest.swift
@@ -545,6 +545,37 @@ class HTTPResponseCompressorTest: XCTestCase {
         XCTAssertNoThrow(try channel.pipeline.removeHandler(name: "compressor").wait())
         XCTAssertNoThrow(try writePromise.futureResult.wait())
     }
+    
+    func testChunkedGzipResponseProducesCorrectNumberOfWrites() throws {
+        let channel = try compressionChannel()
+        try sendRequest(acceptEncoding: "gzip", channel: channel)
+        let finalPromise = channel.eventLoop.makePromise(of: Void.self)
+        let head = HTTPResponseHead(version: HTTPVersion(major: 1, minor: 1), status: .ok)
+        var bodyBuffer = channel.allocator.buffer(capacity: 20)
+        bodyBuffer.writeBytes([UInt8](repeating: 60, count: 20))
+
+        channel.write(NIOAny(HTTPServerResponsePart.head(head)), promise: nil)
+        channel.writeAndFlush(NIOAny(HTTPServerResponsePart.body(.byteBuffer(bodyBuffer))), promise: nil)
+        channel.writeAndFlush(NIOAny(HTTPServerResponsePart.end(nil)), promise: finalPromise)
+        
+        try finalPromise.futureResult.wait()
+        
+        var writeCount = 0
+        while try channel.readOutbound(as: ByteBuffer.self) != nil {
+            writeCount += 1
+        }
+        
+        // Expected number of emitted writes in the chunked response is 8:
+        //   1. HTTP response header
+        //   2. First chunk length
+        //   3. First chunk body
+        //   4. CRLF
+        //   5. Second chunk length
+        //   6. Second chunk body
+        //   7. CRLF
+        //   8. End of message chunk
+        XCTAssertEqual(writeCount, 8)
+    }
 
     func testStartsWithSameUnicodeScalarsWorksOnEmptyStrings() throws {
         XCTAssertTrue("".startsWithSameUnicodeScalars(string: ""))