Add support for compression on the client #707

glbrntt · 2020-01-24T16:01:37Z

Motivation:

Compression support is an optional part of the gRPC specification but
ideally should be supported. Users have also requested this feature
(#611).

Since introducing compression without breaking API is difficult we
should do this before we tag 1.0.0.

Modifications:

Add support for gzip and deflate compression on the client.
Introduce new configuration for supporting compression.
Add interop tests for the client.
Compression is enabled at the connection level (compression used for requests and compression methods to advertise to the server which it may use for responses)
Request compression may be disabled at the RPC level or at the message level for streaming requests.

Result:

Client supports compression.

Motivation: Compression support is an optional part of the gRPC specification but ideally should be supported. Users have also requested this feature (grpc#611). Since introducing compression without breaking API is difficult we should do this before we tag 1.0.0. Modifications: - Add support for gzip and deflate compression on the client. - Introduce new configuration for supporting compression. - Add interop tests for the client. Result: Client supports compression.

Sources/GRPC/HTTP1ToRawGRPCServerCodec.swift

MrMage

Looks good so far. A couple notes:

I have not reviewed Zlib.swift, especially not wrt memory safety and leakage
It looks like the interop tests don't decompress anything; I assume that's expected because the other endpoint would be responsible for that?
You could also add tests for Zlib, but I guess that is integration-tested by CompressorTests.

MrMage · 2020-01-27T09:16:36Z

Sources/GRPC/ClientCalls/ServerStreamingCall.swift

@@ -52,6 +52,7 @@ public final class ServerStreamingCall<RequestMessage: Message, ResponseMessage:
      path: path,
      host: connection.configuration.target.host,
      requestID: requestID,
+      encoding: connection.configuration.compression,


Can we unify the naming of encoding vs compression?

MrMage · 2020-01-27T09:17:15Z

Sources/GRPC/ClientConnection.swift

+    /// `sendMessage(_:disableCompression)`, `sendMessage(_:disableCompression:promise)`,
+    /// `sendMessages(_:disableCompression)` or `sendMessages(_:disableCompression:promise)`.
+    ///
+    /// Note that any disabling compression has no effect if compression is disabled on the


nit: "any disabling compression" sounds weird

Oops, yes, I don't think the any is meant to be there.

MrMage · 2020-01-27T09:17:36Z

Sources/GRPC/ClientOptions.swift

@@ -46,18 +46,24 @@ public struct CallOptions {
  /// messages associated with the call.
  public var requestIDHeader: String?

+  /// Disables request compression on this RPC. Ignored if compression is disabled at the


nit: "on this call"?

Sources/GRPC/Compression/CompressionAlgorithm.swift

MrMage · 2020-01-27T09:20:47Z

Sources/GRPC/Compression/MessageEncoding.swift

+    public internal(set) var outbound: CompressionAlgorithm?
+
+    /// The set of compression algorithms advertised to the remote peer that they may use.
+    public internal(set) var inbound: [CompressionAlgorithm]


Not super happy with these names, neither "requests/responses" nor "outbound/inbound". How about "accepted" and "outbound"? Or something more analogous to the "accept-encoding" headers? Not 100% sure myself. Thinking about this more, maybe the names are actually good.

Also, does gRPC support using different compressions in both directions on one stream? Or is this more like the "preferred" outbound method?

Overall, I'm a bit confused by this so far 😅

Agh...naming here is tricky. To the user requests/responses seems more natural for the init than inbound/outbound, but then responses doesn't quite fit the "accept-encoding" part. Also compression.requests/compression.responses isn't as clear as compression.outbound, compression.inbound.

The init could be more explicit:

init(forRequests outbound: CompressionAlgorithm?, acceptableForResponses inbound: [CompressionAlgorithm]?) { // ... } ... MessageEncoding(forRequests: .gzip, acceptableForResponses: [.gzip, .identity])

I'm still not certain how this API should be spelt! Thoughts?

Also, does gRPC support using different compressions in both directions on one stream? Or is this more like the "preferred" outbound method?

Compression for the request and response stream are independent and may be different.

One other thing I've been considering recently which might make things easier for us and users is some sort of client bootstrap: we can be more descriptive with names and add to it without breaking API (with structs we need a new init if we add a property).

let bootstrap = GRPCConnectionBootstrap(group: group) // hmm sounds familiar... .requestCompression(.gzip) .acceptableResponseCompression(.gzip, .deflate, .identity)

Anyway, that's a story for another time!

MessageEncoding(forRequests: .gzip, acceptableForResponses: [.gzip, .identity]) SGTM. I also like the Bootstrap idea.

I'll leave the bootstrap for another day since that's a fairly large change.

MrMage · 2020-01-27T09:21:00Z

Sources/GRPC/Compression/MessageEncoding.swift

+    public static let none = MessageEncoding(requests: nil, responses: [.identity])
+
+    /// Accept all supported compression on responses, do not compress requests.
+    public static let onlyResponses = MessageEncoding(requests: nil, responses: CompressionAlgorithm.all)


nit: responsesOnly (up to you)

MrMage · 2020-01-27T09:21:16Z

Sources/GRPC/Compression/MessageEncoding.swift

+}
+
+extension ClientConnection.Configuration.MessageEncoding {
+  var acceptEncoding: String {


acceptEncodingHeader?

Sources/GRPC/LengthPrefixedMessageReader.swift

Sources/GRPC/LengthPrefixedMessageWriter.swift

glbrntt

Thanks for the review @MrMage, I'll fix your nits etc when I push up another revision.

Just wanted to check you saw Sources/GRPC/Compression/Zlib.swift? (I noticed there were no comments on it and GitHub hid the diff for me.)

glbrntt · 2020-01-27T13:48:15Z

Sources/GRPC/ClientConnection.swift

+    /// `sendMessage(_:disableCompression)`, `sendMessage(_:disableCompression:promise)`,
+    /// `sendMessages(_:disableCompression)` or `sendMessages(_:disableCompression:promise)`.
+    ///
+    /// Note that any disabling compression has no effect if compression is disabled on the


Oops, yes, I don't think the any is meant to be there.

Sources/GRPC/Compression/CompressionAlgorithm.swift

glbrntt · 2020-01-27T14:01:22Z

Sources/GRPC/Compression/MessageEncoding.swift

+    public internal(set) var outbound: CompressionAlgorithm?
+
+    /// The set of compression algorithms advertised to the remote peer that they may use.
+    public internal(set) var inbound: [CompressionAlgorithm]


Agh...naming here is tricky. To the user requests/responses seems more natural for the init than inbound/outbound, but then responses doesn't quite fit the "accept-encoding" part. Also compression.requests/compression.responses isn't as clear as compression.outbound, compression.inbound.

The init could be more explicit:

init(forRequests outbound: CompressionAlgorithm?, acceptableForResponses inbound: [CompressionAlgorithm]?) { // ... } ... MessageEncoding(forRequests: .gzip, acceptableForResponses: [.gzip, .identity])

I'm still not certain how this API should be spelt! Thoughts?

Also, does gRPC support using different compressions in both directions on one stream? Or is this more like the "preferred" outbound method?

Compression for the request and response stream are independent and may be different.

Sources/GRPC/LengthPrefixedMessageReader.swift

glbrntt · 2020-01-27T14:12:07Z

Sources/GRPC/Compression/MessageEncoding.swift

+    public internal(set) var outbound: CompressionAlgorithm?
+
+    /// The set of compression algorithms advertised to the remote peer that they may use.
+    public internal(set) var inbound: [CompressionAlgorithm]


One other thing I've been considering recently which might make things easier for us and users is some sort of client bootstrap: we can be more descriptive with names and add to it without breaking API (with structs we need a new init if we add a property).

let bootstrap = GRPCConnectionBootstrap(group: group) // hmm sounds familiar... .requestCompression(.gzip) .acceptableResponseCompression(.gzip, .deflate, .identity)

Anyway, that's a story for another time!

MrMage

Looks good overall, have given Zlib.swift a cursory look overall now. (No time to study the Zlib docs.)

MrMage · 2020-01-28T13:02:23Z

Sources/GRPC/Compression/Zlib.swift

+      self.end()
+    }
+
+    /// Decompresses the data in `input` into the `output` buffer.


Wait, shouldn't this method be about compressing?

Good catch, thanks!

Sources/GRPC/Compression/Zlib.swift

glbrntt · 2020-01-28T14:20:12Z

Missed this before:

It looks like the interop tests don't decompress anything; I assume that's expected because the other endpoint would be responsible for that?

They do decompress in the ServerCompressed{Unary,Streaming} tests (client sends uncompressed data, server responds with compressed data).

You could also add tests for Zlib, but I guess that is integration-tested by CompressorTests.

That's right. I didn't think there was much value in adding them.

MrMage

LGTM

Sources/GRPC/Compression/Zlib.swift

glbrntt · 2020-01-29T12:39:27Z

@MrMage just pushed up a small patch, the compression documentation isn't clear in some places but working on the server part of this made me realise that something wasn't quite right. Here's the change: ffe5bd4

MrMage · 2020-01-29T12:51:52Z

Still LGTM :-)

…

On 29. Jan 2020, at 13:39, George Barnett ***@***.***> wrote: @MrMage just pushed up a small patch, the compression documentation isn't clear in some places but working on the server part of this made me realise that something wasn't quite right. Here's the change: ffe5bd4 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

glbrntt requested a review from MrMage January 24, 2020 16:01

glbrntt commented Jan 24, 2020

View reviewed changes

Sources/GRPC/HTTP1ToRawGRPCServerCodec.swift Show resolved Hide resolved

Update LinuxMain

7bb2659

MrMage reviewed Jan 27, 2020

View reviewed changes

glbrntt commented Jan 27, 2020

View reviewed changes

glbrntt added 11 commits January 27, 2020 14:22

Add missing copyright header

2e5c329

Rename CompressorTests

ee9ca0a

Rename compression to messageEncoding in ClientConnection.Configuration

2f228ad

remove extra "any"

9674f94

rename RPC to call in CallOptions documentation

76588db

rename onlyResponses to responsesOnly

185babc

rename acceptEncoding to acceptEncodingHeader

e9b790f

More comments in the message writer

ff34c8b

onlyResponses -> responsesOnly

db6e85b

give swift 5 a helping hand when switching over optionals

6fc3365

add a few more tests

6b51a9f

MrMage reviewed Jan 28, 2020

View reviewed changes

glbrntt added 3 commits January 28, 2020 13:49

Update labels for MessageEncoding

40d3ad7

fixup deflate documentation

fb0a2f0

Add some documentation to ZStream

3270253

MrMage approved these changes Jan 28, 2020

View reviewed changes

Sources/GRPC/Compression/Zlib.swift Show resolved Hide resolved

fixup incorrect assumption

ffe5bd4

glbrntt merged commit 151b627 into grpc:nio Jan 29, 2020

glbrntt deleted the gb-compression branch January 29, 2020 12:54

glbrntt added the ⚠️ semver/major Breaks existing public API. label Feb 7, 2020

uchiha-itachi95 mentioned this pull request Dec 20, 2021

Example code to enable compression and decompression on client side ? #1337

Closed

Add support for compression on the client #707

Add support for compression on the client #707

Uh oh!

Conversation

glbrntt commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MrMage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrMage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glbrntt commented Jan 28, 2020

Uh oh!

MrMage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glbrntt commented Jan 29, 2020

Uh oh!

MrMage commented Jan 29, 2020 via email

Uh oh!

Uh oh!

glbrntt commented Jan 24, 2020 •

edited

Loading