Adds the ability to use different payloads #710

mustiikhalil · 2020-01-28T07:55:17Z

Good morning!

The following PR will be moving the Message Protocol away from the code base and introduces the Payload protocol which will allow users, to decode messages according to the Type they want to send. it addresses the following issue: #708, @glbrntt let's see how we can make this better since I found the following issue:

issues:
Adds an over head of the code gen since now you will have to add the

extension Helloworld_HelloReply: Payload {
    public init(serializedByteBuffer: inout ByteBuffer) throws {
        try self.init(serializedData: serializedByteBuffer.readData(length: serializedByteBuffer.readableBytes)!,
                  extensions: nil,
                  partial: false,
                  options: BinaryDecodingOptions())
    }
    
    public func serialize(into buffer: inout ByteBuffer) throws {
        let data = try serializedData()
        buffer.writeBytes(data)
    }
}

which is not needed all the time since the following object didn't require it to work

extension Routeguide_RouteNote: Payload {
    public init(serializedByteBuffer: inout ByteBuffer) throws {}
    public func serialize(into buffer: inout ByteBuffer) throws {}
}

linux-foundation-easycla · 2020-01-28T07:55:20Z

The committers are authorized under a signed CLA.

✅ mustiikhalil (f5e9f93, ba22a7c)

mustiikhalil · 2020-01-28T08:03:20Z

Sources/GRPC/ReadWriteStates.swift

+      // Zero is fine: the writer will allocate the correct amount of space.
+      var buffer = allocator.buffer(capacity: 0)
+
+      do {
+        try message.serialize(into: &buffer)
+      } catch {
        self = .notWriting
        return .failure(.serializationFailed)
      }
-
-      // Zero is fine: the writer will allocate the correct amount of space.
-      var buffer = allocator.buffer(capacity: 0)
+      let data = buffer.readData(length: buffer.readableBytes)!
+      buffer.clear()
      writer.write(data, into: &buffer)


is there a prettier way of doing this?

Yes, but unfortunately we need to rework the writer API slightly. I have a PR up at the moment which adds support for compression which complicates things a little more. For the time being I think making the writer generic over the payload and having it serialize the payload into the buffer is sufficient.

glbrntt

Great start @mustiikhalil! I've left some comments inline.

You may already be pretty close to the point where you can try out flatbuffers. If you look at e.g, the hello world example and switch the types to be flatbuffers and provide the conformance to Payload then it should work. Of course this is still missing all the flatbuffers codegen :)

Sources/GRPC/Payload.swift

glbrntt · 2020-01-28T10:21:56Z

Sources/GRPC/ReadWriteStates.swift

+      // Zero is fine: the writer will allocate the correct amount of space.
+      var buffer = allocator.buffer(capacity: 0)
+
+      do {
+        try message.serialize(into: &buffer)
+      } catch {
        self = .notWriting
        return .failure(.serializationFailed)
      }
-
-      // Zero is fine: the writer will allocate the correct amount of space.
-      var buffer = allocator.buffer(capacity: 0)
+      let data = buffer.readData(length: buffer.readableBytes)!
+      buffer.clear()
      writer.write(data, into: &buffer)


Yes, but unfortunately we need to rework the writer API slightly. I have a PR up at the moment which adds support for compression which complicates things a little more. For the time being I think making the writer generic over the payload and having it serialize the payload into the buffer is sufficient.

glbrntt · 2020-01-28T10:38:45Z

Sources/Examples/Echo/Model/echo.grpc.swift

  }
 }

+extension Echo_EchoRequest: Payload {


This file is generated, but it looks like these were added manually.

The implementation for the conformance to Payload shouldn't be in the generated code. Instead we can create another protocol (e.g. GRPCProtobufPayload) which conforms to Payload and Message. It should then provide the default implementation for Payload so that the generated messages can be extended to conform to that protocol.

You'll need to take a look at the code-gen (in protoc-gen-grpc-swift) to figure out how to do this.

@glbrntt Yes, I just added them to see if they would pass the test cases (since they are still failing) and to have a draft of the PR. So we can talk about What we would be doing, and how.

we can have the following:

protocol GRPCProtobufPayload: GRPCPayload {} extension GRPCProtobufPayload { // comes the implementation of the functions }

and similarly in FlatBuffers:

protocol GRPCFlatbufPayload: GRPCPayload {} extension GRPCFlatbufPayload { // comes the implementation of the functions }

do you think we should generate the code? or implement it which the package itself?

Ah okay, no problem. Just an FYI if you want to run the tests locally you can set the ENABLE_TIMING_TESTS environment variable to false to skip some of the longer running tests (it takes the test time down to a couple of seconds).

The default implementation for GRPCProtobufPayload (which should also conform to SwiftProtobuf.Message) should be within GRPC since we already depend on SwiftProtobuf. This gives us more flexibility to change the implementation (i.e. if/when new Protobuf APIs become available) without regenerating the code.

For flatbuffers I think we have to generate the conformance to GRPCPayload each time since there's no dependency between flatbuffers and gRPC Swift here.

Sources/GRPC/Payload.swift

Sources/GRPC/GRPCPayload.swift

Sources/GRPC/GRPCProtobufPayload.swift

Sources/GRPC/GRPCPayload.swift

Sources/GRPC/GRPCProtobufPayload.swift

mustiikhalil · 2020-01-29T15:04:37Z

@glbrntt I think this is ready for review now! Thanks a million for the guidance though, did learn new things!

glbrntt

Thanks for this @mustiikhalil, I've left some comments inline.

The CI is failing because the committed code for the Echo service doesn't match the code generated for that service; you'll need to regenerate it and commit it again.

Sources/GRPC/CallHandlers/BidirectionalStreamingCallHandler.swift

Sources/GRPC/GRPCPayload.swift

Sources/GRPC/GRPCProtobufPayload.swift

glbrntt · 2020-01-30T10:42:48Z

Sources/GRPC/GRPCServerCodec.swift

+        var buffer = ByteBufferAllocator().buffer(capacity: 0)
+        try message.serialize(into: &buffer)
+        context.write(self.wrapOutboundOut(.message(buffer.readData(length: buffer.readableBytes)!)), promise: promise)


Because of this change, we have quite a few unnecessary copies here and on the client side (between Data and ByteBuffer). This might require a fair bit of refactoring; would you be interested in doing this? If not I'm happy to do this in a followup.

I would gladly do it but do you have a structure in mind or should I look into how we can do it?

Sure, I'll try to explain the problem and what needs to be done to improve it.

Problem

As you know, before this change we explicitly expected payloads to be Message from SwiftProtobuf. The only serialization and deserialization methods from Message are via Data from Foundation. Internally we use NIO and our currency type for storing bytes is NIOs ByteBuffer.

Because we always expect a Data from Message, the write method on LengthPrefixedMessageWriter accepts a Data and writes into a ByteBuffer.

With the protocol we always serialize into a ByteBuffer (for Message we have to go via Data first). This means that we've gone from:

Message to Data (serialization)

Data to ByteBuffer (length prefixing)

to

Message to Data (serialization)

Data to ByteBuffer (we need to for GRPCPayload)

ByteBuffer to Data (we need to for LengthPrefixedMessageWriter.write)

Data to ByteBuffer (length prefixing)

Clearly we don't want to copy bytes from Data to ByteBuffer and back from ByteBuffer to Data again (and we also don't want our API for GRPCPayload to be defined by Data).

Now I need to provide some background on the LenthPrefixedMessageWriter: gRPC payloads are sent with a 5-byte prefix (I might have explained this before but I'll repeat it for completeness). The first byte indicates whether the payload is compressed or not, the next four bytes indicate the length of the payload (the compressed length if it is compressed).

If the payload isn't compressed then this is actually really straightforward: we write a 0-byte we write the length of the payload as a UInt32 and then we write the payload. For compression we have to write the a 1-byte, leave a 4-byte gap for the length, compress the payload into the buffer and not how many bytes it was and then fill in the 4-byte gap.

Possible Solution

So I think the best we can do now (I'll use protobuf as an example again) is actually to pre-allocate the extra 5-byte space at the beginning of the buffer before we pass it into message.serialize(into:).

If we aren't using compression then all we need to do is fill in the first 5 bytes, easy! (And we have the Message to Data to ByteBuffer, as we had before, for Flatbuffers I expect we have something similar.)

When we use compression things are slightly different because we can't compress in place, so we have an extra buffer but in this case the same applies, we can avoid the extra hop via Data.

Hopefully this all makes sense so far?

What needs to be done?

LengthPrefixedMessageWriter.write accepts a Data, instead I think the writer should accept a GRPCPayload (as the writer knows whether compression is being used so knows how to fill in the first 5-bytes of the payload etc, it can also choose whether it should preallocate 5 bytes, if we're using compression then we don't need to since we have to compress into a different buffer anyway).

Zlib.deflate also accepts a Data if I recall correctly, this will need to be changed to a ByteBuffer

On the server side of things, this will require having to move some components around. At the moment the length prefixed writer lives in HTTP1ToRawGRPCServerCodec; this will have to move to GPRCServerCodec. This means the types that get sent between these handlers will also have to change (_RawGRPCServerResponsePart.message will have a ByteBuffer as its payload which will be a length prefixed message). I also expect a number of tests will have to be updated to support these changes.

Sources/protoc-gen-grpc-swift/Generator.swift

Sources/GRPC/GRPCProtobufPayload.swift

Sources/GRPC/GRPCPayload.swift

Sources/GRPC/GRPCProtobufPayload.swift

mustiikhalil · 2020-01-31T11:30:00Z

I've fixed all the suggested things, and also refactored the code according to what we planned yesterday, however there is a point in the HTTP1ToRawGRPCServerCodec where I just disregarded the first 5 bytes in the buffer, which I really hate doing, but I simply don't fully understand the library to come up with a solution.

// we do this since Foundation data used to come without the first 5 bytes
var messageBytes = messageByte
// disregards the first 5 bytes since the rewritting them to the buffer doesnt require them
messageBytes.moveReaderIndex(forwardBy: protobufMetadataSize)

Furthermore, I ran both commands locally and there is no difference between both I am not sure why the CLI is having issues with it :/
(Yes, the generated file is built by the cli)

protoc Sources/Examples/Echo/Model/echo.proto \                                                                                                              --proto_path=./Sources/Examples/Echo/Model/ \
--plugin=./.build/release/protoc-gen-grpc-swift \
--grpc-swift_opt=Visibility=Public \
--grpc-swift_out=.

diff -u ./echo.grpc.swift Sources/Examples/Echo/Model/echo.grpc.swift

glbrntt · 2020-01-31T14:54:22Z

Great, I'll take another look now.

Furthermore, I ran both commands locally and there is no difference between both I am not sure why the CLI is having issues with it :/
(Yes, the generated file is built by the cli)
protoc Sources/Examples/Echo/Model/echo.proto \                                                                                                              --proto_path=./Sources/Examples/Echo/Model/ \
--plugin=./.build/release/protoc-gen-grpc-swift \
--grpc-swift_opt=Visibility=Public \
--grpc-swift_out=.

diff -u ./echo.grpc.swift Sources/Examples/Echo/Model/echo.grpc.swift

Oh I think I see the problem, the CI looks like it isn't actually rebuilding the plugin (maybe the workspace gets cached between different commits for the same PR?!). If you update the rule in the Makefile for building the plugin (line 34) to be the following then it should work 🤞:

${PROTOC_GEN_GRPC_SWIFT}: Sources/protoc-gen-grpc-swift/*.swift

mustiikhalil · 2020-01-31T15:16:21Z

let's see hopefully it works.
Rebased let's see if that will help too

… with GRPC-swift Renames all the Requests and Responses + adds comments + fixes generator code Updated the code to only use bytebuffer instead of foundation data Move the logic for reading the payload to GRPCServerCodec, and fixed the logic Updated the rule for travis ci

glbrntt

I've left some more comments @mustiikhalil -- great work!

Sources/GRPC/Compression/Zlib.swift

Sources/GRPC/GRPCPayload.swift

Sources/GRPC/GRPCProtobufPayload.swift

Sources/GRPC/LengthPrefixedMessageWriter.swift

Sources/GRPC/ReadWriteStates.swift

Sources/GRPCInteroperabilityTestModels/Generated/test.grpc.swift

Sources/GRPC/LengthPrefixedMessageWriter.swift

Sources/GRPC/GRPCServerCodec.swift

…ming + generated code

glbrntt

A few more changes needed but this is getting pretty close though @mustiikhalil.

Sources/GRPC/Compression/Zlib.swift

Sources/GRPC/LengthPrefixedMessageWriter.swift

Sources/protoc-gen-grpc-swift/Generator.swift

Sources/GRPC/HTTP1ToRawGRPCServerCodec.swift

Sources/GRPC/GRPCPayload.swift

mustiikhalil · 2020-02-03T11:17:11Z

Sources/GRPC/Compression/Zlib.swift

        }
+
+        let bytesRead = inputPointer.count - self.stream.availableInputBytes
+        return (bytesRead, writtenBytes)


@glbrntt I think this should fix the issue with the pointer being accessed while we already moved out of the function

mustiikhalil · 2020-02-03T15:00:59Z

Sources/GRPC/LengthPrefixedMessageWriter.swift

-    buffer.reserveCapacity(MemoryLayout.size(ofValue: payload) + LengthPrefixedMessageWriter.metadataLength)
-
+  func write(_ payload: GRPCPayload, into buffer: inout ByteBuffer, disableCompression: Bool = false) throws {r
+    buffer.reserveCapacity(buffer.writerIndex + LengthPrefixedMessageWriter.metadataLength)


moved the code to use the buffer.writerIndex instead of the MemoryLayout.size(of: payload) since I think this way it will be more robust knowing that it will always have those 5 extra bytes on the buffer.

Great catch, thanks!

glbrntt

Looks good to me, thanks for sticking with this @mustiikhalil; great job!

glbrntt · 2020-02-04T11:54:36Z

Sources/GRPC/LengthPrefixedMessageWriter.swift

-    buffer.reserveCapacity(MemoryLayout.size(ofValue: payload) + LengthPrefixedMessageWriter.metadataLength)
-
+  func write(_ payload: GRPCPayload, into buffer: inout ByteBuffer, disableCompression: Bool = false) throws {r
+    buffer.reserveCapacity(buffer.writerIndex + LengthPrefixedMessageWriter.metadataLength)


Great catch, thanks!

mustiikhalil · 2020-02-04T13:15:04Z

Wonderful!

mustiikhalil commented Jan 28, 2020

View reviewed changes

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from cb2755e to e4a5737 Compare January 28, 2020 08:06

mustiikhalil changed the title ~~Add ability to use different payloads~~ Adds the ability to use different payloads Jan 28, 2020

glbrntt requested changes Jan 28, 2020

View reviewed changes

mustiikhalil commented Jan 28, 2020

View reviewed changes

Sources/GRPC/GRPCPayload.swift Show resolved Hide resolved

mustiikhalil commented Jan 28, 2020

View reviewed changes

Sources/GRPC/GRPCProtobufPayload.swift Outdated Show resolved Hide resolved

glbrntt reviewed Jan 29, 2020

View reviewed changes

Sources/GRPC/GRPCPayload.swift Show resolved Hide resolved

Sources/GRPC/GRPCProtobufPayload.swift Outdated Show resolved Hide resolved

Sources/GRPC/GRPCProtobufPayload.swift Outdated Show resolved Hide resolved

Sources/GRPC/GRPCProtobufPayload.swift Outdated Show resolved Hide resolved

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from 9439f1e to 8827fca Compare January 29, 2020 14:24

mustiikhalil marked this pull request as ready for review January 29, 2020 15:03

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch 3 times, most recently from b045bce to 287022f Compare January 29, 2020 16:39

glbrntt requested changes Jan 30, 2020

View reviewed changes

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch 3 times, most recently from 167989f to 75f3133 Compare January 31, 2020 11:19

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from 98ef91b to 19f2604 Compare January 31, 2020 15:27

glbrntt requested changes Jan 31, 2020

View reviewed changes

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch 3 times, most recently from 3f8effa to 00f9f91 Compare January 31, 2020 20:14

Addresses some of the comments in the PR regarding documentation + na…

ec0d8b6

…ming + generated code

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch 2 times, most recently from 95bcae7 to b180af7 Compare January 31, 2020 22:46

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch 3 times, most recently from 1135358 to ff56fe4 Compare February 1, 2020 11:35

Refactors code in LengthPrefixedMessageWritter and zlib

1cfb34c

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from ff56fe4 to 1cfb34c Compare February 1, 2020 11:36

glbrntt requested changes Feb 3, 2020

View reviewed changes

Improves documentation + fixes some issues

ed13479

mustiikhalil commented Feb 3, 2020

View reviewed changes

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from df14977 to 9ffa268 Compare February 3, 2020 14:57

mustiikhalil commented Feb 3, 2020

View reviewed changes

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from 9ffa268 to c94ca28 Compare February 3, 2020 15:02

Fixes zlib function + write function

052f513

mustiikhalil force-pushed the add-ability-to-use-different-payloads branch from c94ca28 to 052f513 Compare February 3, 2020 15:17

glbrntt approved these changes Feb 4, 2020

View reviewed changes

glbrntt merged commit 86c421e into grpc:nio Feb 4, 2020

mustiikhalil deleted the add-ability-to-use-different-payloads branch February 4, 2020 13:15

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

Adds the ability to use different payloads #710

Adds the ability to use different payloads #710

Uh oh!

Conversation

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

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glbrntt left a comment

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!

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

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Problem

Possible Solution

What needs to be done?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

glbrntt commented Jan 31, 2020

Uh oh!

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

Uh oh!

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mustiikhalil commented Jan 28, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jan 28, 2020 •

edited

Loading

mustiikhalil Jan 28, 2020 •

edited

Loading

mustiikhalil commented Jan 29, 2020 •

edited

Loading

mustiikhalil commented Jan 31, 2020 •

edited

Loading

mustiikhalil commented Jan 31, 2020 •

edited

Loading