Support grpc-web-text #2938

minwoox · 2020-07-27T06:17:27Z

Motivation:
Currently, we only support grpc-web, not grpc-web-text.
It would be nice if we support both.

Modifications:

GrpcService now supports all GrpcSerializationFormats by default.
ArmeriaMessageFramer and ArmeriaMessageDeframer now takes the additional boolean flag which indicates encoding decoding base64
Add gRPC-web it test with akka-grpc
- The official grpc-java does not support grpc-web so we use akka-grpc for it testing

Result:

You can now use grpc-web-text using Armeria client and server.
Close gRPC web text support #2911

TO-do:

Support trailer parsing in GrpcWebUtil.

Motivation: Currently, we only support grpc-web, not grpc-web-text. It would be nice if we support both. Modifications: - `ArmeriaMessageFramer` and `ArmeriaMessageDeframer` now takes the additional boolean flag which indicates encoding decoding base64 - Add gRPC-web it test with akka-grpc - The official grpc-java does not support grpc-web so we use akka-grpc for it testing Result: - You can now use grpc-web-text using Armeria client and server.

...protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageDeframer.java

codecov · 2020-07-27T06:36:04Z

Codecov Report

Merging #2938 into master will decrease coverage by 0.01%.
The diff coverage is 84.21%.

@@             Coverage Diff              @@
##             master    #2938      +/-   ##
============================================
- Coverage     73.22%   73.21%   -0.02%     
- Complexity    12198    12209      +11     
============================================
  Files          1061     1062       +1     
  Lines         47273    47343      +70     
  Branches       5967     5979      +12     
============================================
+ Hits          34614    34660      +46     
- Misses         9643     9656      +13     
- Partials       3016     3027      +11

Impacted Files	Coverage Δ	Complexity Δ
...inecorp/armeria/client/grpc/GrpcClientOptions.java	`100.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...corp/armeria/unsafe/grpc/GrpcUnsafeBufferUtil.java	`73.33% <0.00%> (-5.24%)`	`4.00 <0.00> (ø)`
.../com/linecorp/armeria/client/grpc/GrpcWebUtil.java	`66.66% <60.00%> (-2.09%)`	`2.00 <2.00> (ø)`
...ia/internal/common/grpc/GrpcMessageMarshaller.java	`64.48% <66.66%> (-1.23%)`	`18.00 <0.00> (ø)`
...rmeria/internal/client/grpc/ArmeriaClientCall.java	`81.34% <77.77%> (+0.19%)`	`48.00 <6.00> (ø)`
...inecorp/armeria/server/grpc/ArmeriaServerCall.java	`87.22% <83.33%> (+1.00%)`	`77.00 <0.00> (-1.00)`	⬆️
...rp/armeria/common/grpc/protocol/Base64Decoder.java	`84.61% <84.61%> (ø)`	`9.00 <9.00> (?)`
.../armeria/client/grpc/protocol/UnaryGrpcClient.java	`77.04% <100.00%> (ø)`	`8.00 <0.00> (ø)`
...a/common/grpc/protocol/ArmeriaMessageDeframer.java	`73.68% <100.00%> (+1.13%)`	`51.00 <0.00> (+3.00)`
...ria/common/grpc/protocol/ArmeriaMessageFramer.java	`93.65% <100.00%> (+1.05%)`	`25.00 <1.00> (+3.00)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23b89a5...d13089d. Read the comment docs.

gradle/scripts/lib/java-rpc-proto.gradle

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageFramer.java

dependencies.yml

Related: line/armeria#2938

trustin

How about updating the *-grpc.mdx?

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageFramer.java

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/Base64Decoder.java

grpc-protocol/src/test/java/com/linecorp/armeria/common/grpc/protocol/Base64DecoderTest.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcWebUtil.java

minwoox · 2020-07-29T03:36:40Z

How about updating the *-grpc.mdx?

Thanks!, updated client-grpc.mdx.
I think server-grpc.mdx doesn't need a change so I just left it. 😉

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageFramer.java

grpc-protocol/src/test/java/com/linecorp/armeria/common/grpc/protocol/Base64DecoderTest.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcWebUtil.java

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/Base64Decoder.java

grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcWebUtil.java

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java

grpc/src/main/java/com/linecorp/armeria/server/grpc/ArmeriaServerCall.java

site/src/pages/docs/client-grpc.mdx

minwoox · 2020-07-29T09:21:44Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java

@@ -65,7 +65,7 @@
 public final class GrpcServiceBuilder {

    private static final Set<SerializationFormat> DEFAULT_SUPPORTED_SERIALIZATION_FORMATS =
-            ImmutableSet.of(GrpcSerializationFormats.PROTO, GrpcSerializationFormats.PROTO_WEB);
+            GrpcSerializationFormats.values();


Changed to support all GrpcSerializationFormats by default because there's no regression on performance and @ikhoon sometimes forgets about specifying JSON format when he's doing some test. 🤣 /cc @anuraaga

ikhoon · 2020-07-29T09:22:47Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java

-            ImmutableSet.of(GrpcSerializationFormats.PROTO, GrpcSerializationFormats.PROTO_WEB);
+            ImmutableSet.of(GrpcSerializationFormats.PROTO,
+                            GrpcSerializationFormats.PROTO_WEB,
+                            GrpcSerializationFormats.PROTO_WEB_TEXT);


Could you enable all gRPC protocols by default?
Most of Armeria users use gRPC-Java and com.google.protobuf.Message. Armeria can encode and decode the messages by default.

Even though Armeria does not support ScalaPB JSON by default. 😅
Any thoughts? @anuraaga

Oops... Ditto 🤣

grpc/src/main/java/com/linecorp/armeria/unsafe/grpc/GrpcUnsafeBufferUtil.java

trustin

Finally! 🎉

ikhoon

Nice work! @minwoox

anuraaga · 2020-07-30T04:11:13Z

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/Base64Decoder.java

+import io.netty.util.ByteProcessor;
+
+/**
+ * A stateful Base64decoder. Unlike {@link Base64#getDecoder()}, this decoder does not end when it meets


Suggested change

* A stateful Base64decoder. Unlike {@link Base64#getDecoder()}, this decoder does not end when it meets

* A stateful Base64decoder. Unlike {@link io.netty.handler.codec.base64.Base64Decoder}, this decoder does not end when it meets

Important to describe the difference in this fork, not behavior vs JDK.

anuraaga · 2020-07-30T04:12:28Z

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/Base64Decoder.java

+
+        // White space, Equals sign or better
+        if (decodedByte < EQUALS_SIGN_ENC) {
+            // Ignore the white space.


Why do we ignore white space, that seems like malformed base64?

Let me raise an exception. 😄

anuraaga · 2020-07-30T04:17:18Z

grpc-protocol/src/test/java/com/linecorp/armeria/common/grpc/protocol/Base64DecoderTest.java

+        decodedSecond.release();
+    }
+
+    private static List<ByteBuf> fragmentRandomly(byte[] bytes) {


I don't think we should generate test cases randomly, it's very hard to be convinced this class is actually a complete test for base64, can you write a more concrete, easy to reason about test suite? Of course this is a big reason I didn't want to implement it :)

I'm working on a PR that supports compression and decoding base64 in GrpcWebUtil.
Let me work on this in that PR. 😄

trustin · 2020-07-30T04:20:53Z

@anuraaga Please keep commenting. We'll send a follow up PR for them. 🙇

@anuraaga

Address the comments from @anuraaga

@anuraaga

Motivation: The [spec](https://tools.ietf.org/html/rfc4648#section-3.1) says that ``` Implementations MUST NOT add line feeds to base-encoded data unless the specification referring to this document explicitly directs base encoders to add line feeds after a specific number of characters. ``` So we should not add line-feed when base64 encoding. Motivations: - Do not add line-feed when base64 encoding. - Address comments from @anuraaga in #2938. Result: - grpc-web-text does not contain line-feed characters.

Motivation: I made a mistake in line#2938 that the `ByteBufInputStream` is not closed. Modification: - Close `ByteBufInputStream` Result: - Less leaks

Motivation: I made a mistake in #2938 that the `ByteBufInputStream` is not closed. Modification: - Close `ByteBufInputStream` Result: - Less leaks

Motivation: Currently, we only support grpc-web, not grpc-web-text. It would be nice if we support both. Modifications: - `GrpcService` now supports all `GrpcSerializationFormats` by default. - `ArmeriaMessageFramer` and `ArmeriaMessageDeframer` now takes the additional boolean flag which indicates encoding decoding base64 - Add gRPC-web it test with akka-grpc - The official grpc-java does not support grpc-web so we use akka-grpc for it testing Result: - You can now use grpc-web-text using Armeria client and server. - Close line#2911

@anuraaga

…e#2946) Motivation: The [spec](https://tools.ietf.org/html/rfc4648#section-3.1) says that ``` Implementations MUST NOT add line feeds to base-encoded data unless the specification referring to this document explicitly directs base encoders to add line feeds after a specific number of characters. ``` So we should not add line-feed when base64 encoding. Motivations: - Do not add line-feed when base64 encoding. - Address comments from @anuraaga in line#2938. Result: - grpc-web-text does not contain line-feed characters.

Motivation: I made a mistake in line#2938 that the `ByteBufInputStream` is not closed. Modification: - Close `ByteBufInputStream` Result: - Less leaks

minwoox added the new feature label Jul 27, 2020

minwoox added this to the 0.99.9 milestone Jul 27, 2020

minwoox requested review from trustin, anuraaga and ikhoon July 27, 2020 06:17

anuraaga reviewed Jul 27, 2020

View reviewed changes

...protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageDeframer.java Outdated Show resolved Hide resolved

trustin reviewed Jul 27, 2020

View reviewed changes

gradle/scripts/lib/java-rpc-proto.gradle Outdated Show resolved Hide resolved

trustin reviewed Jul 27, 2020

View reviewed changes

grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaMessageFramer.java Show resolved Hide resolved

ikhoon reviewed Jul 27, 2020

View reviewed changes

dependencies.yml Outdated Show resolved Hide resolved

Support spanned encoded data

df84329

minwoox mentioned this pull request Jul 28, 2020

Add akka-grpc-codegen for akka protobuf line/gradle-scripts#78

Merged

minwoox added 3 commits July 28, 2020 17:59

Optimize base 64 decoder thanks to @trustin

c13c6c2

Revert GrpcWebUtil

0f4edb0

Update rpc gradle

a340f2d

trustin pushed a commit to line/gradle-scripts that referenced this pull request Jul 28, 2020

Add akka-grpc-codegen for akka protobuf (#78)

10d2ca8

Related: line/armeria#2938

trustin reviewed Jul 29, 2020

View reviewed changes

Address the comments by @trustin

fbfe8c5

trustin reviewed Jul 29, 2020

View reviewed changes

Address the comments by @trustin

f8c00ae

ikhoon reviewed Jul 29, 2020

View reviewed changes

Support all serialization formats by default

9d9cad5

minwoox commented Jul 29, 2020

View reviewed changes

ikhoon reviewed Jul 29, 2020

View reviewed changes

trustin reviewed Jul 29, 2020

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/unsafe/grpc/GrpcUnsafeBufferUtil.java Outdated Show resolved Hide resolved

minwoox added 3 commits July 29, 2020 18:37

Address comments by @ikhoon

9b5fb9b

Release silently

70a7fbd

Update documenation

6478d30

trustin approved these changes Jul 29, 2020

View reviewed changes

minwoox added 3 commits July 30, 2020 12:26

Use scala 2.13

fb738fd

Use scala version in flag

1f35298

Merge branch 'master' into grpc-web-text

30da9cf

ikhoon approved these changes Jul 30, 2020

View reviewed changes

Merge branch 'master' into grpc-web-text

d13089d

trustin merged commit 878684c into line:master Jul 30, 2020

anuraaga reviewed Jul 30, 2020

View reviewed changes

minwoox deleted the grpc-web-text branch July 30, 2020 04:29

minwoox added a commit to minwoox/armeria that referenced this pull request Jul 30, 2020

Follow up for line#2938

d4ef0cb

Address the comments from @anuraaga

minwoox mentioned this pull request Jul 30, 2020

Fix a bug where grpc-web-text add line-feed when base64 encoding #2946

Merged

minwoox added a commit to minwoox/armeria that referenced this pull request Jul 31, 2020

Fix a leak in ArmeriaClientCall

a99e2a4

Motivation: I made a mistake in line#2938 that the `ByteBufInputStream` is not closed. Modification: - Close `ByteBufInputStream` Result: - Less leaks

minwoox mentioned this pull request Jul 31, 2020

Fix a leak in ArmeriaClientCall #2955

Merged

trustin pushed a commit that referenced this pull request Jul 31, 2020

Fix a leak in ArmeriaClientCall (#2955)

f3614fc

Motivation: I made a mistake in #2938 that the `ByteBufInputStream` is not closed. Modification: - Close `ByteBufInputStream` Result: - Less leaks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support grpc-web-text #2938

Support grpc-web-text #2938

minwoox commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading

trustin left a comment

minwoox commented Jul 29, 2020

minwoox Jul 29, 2020

ikhoon Jul 29, 2020

ikhoon Jul 29, 2020

trustin left a comment

ikhoon left a comment

anuraaga Jul 30, 2020

anuraaga Jul 30, 2020

minwoox Jul 30, 2020

anuraaga Jul 30, 2020

minwoox Jul 30, 2020

trustin commented Jul 30, 2020

	* A stateful Base64decoder. Unlike {@link Base64#getDecoder()}, this decoder does not end when it meets
	* A stateful Base64decoder. Unlike {@link io.netty.handler.codec.base64.Base64Decoder}, this decoder does not end when it meets

Support grpc-web-text #2938

Support grpc-web-text #2938

Conversation

minwoox commented Jul 27, 2020 • edited Loading

codecov bot commented Jul 27, 2020 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon 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

trustin commented Jul 30, 2020

minwoox commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading