Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure #4033

jrhee17 · 2022-01-16T15:13:17Z

Motivation:

Currently, armeria server fails to start up while trying to create a MessageMarshaller for proto2 files containing Enum definitions.

It seems like protobuf-jackson doesn't support proto2.
However, it seems like upstream at least partially supports proto2.

https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/util/JsonFormat

The JSON format follows Proto3 JSON specification and only proto3 features are supported. Proto2 only features (e.g., extensions and unknown fields) will be discarded in the conversion. That is, when converting proto2 messages to JSON format, extensions and unknown fields will be treated as if they do not exist. This applies to proto2 messages embedded in proto3 messages as well.

~~I propose that we fallback to use the upstream implementation as the default json marshaller when a service includes any proto2 defined message.~~
I realized that it is difficult to support such default implementation since

It isn't possible to detect if proto2 specific features are used. Even though proto2 files are included, users may not be using proto2 specific features. These users will incur the penalty of using the gson implementation unnecessarily.
Since GrpcJsonMarshallerBuilder#jsonMarshallerCustomizer is exposed, it is difficult to introduce a new default json marshalling implementation without breaking changes. (we will need to add a mapping from MessageMarshaller#Builder to JsonFormat options which probably isn't generic/worth it)

Considering the above, I believe it is easier just provide an option so that users can opt-in for gson serde

Modifications:

Introduce a GsonGrpcJsonMarshaller which performs json marshalling using google's apis. Also introduce a corresponding builder class GrpcJsonMarshallerBuilder
Add apis GrpcJsonMarshaller#ofJackson, GrpcJsonMarshaller#builderForJackson, GrpcJsonMarshaller#ofGson, GrpcJsonMarshaller#builderForGson

Result:

Closes Server fails to start with a grpc service for a proto2 proto that contains an enum. #4020
Armeria now provides an easy way to fall back to the upstream gson implementation

codecov · 2022-01-16T16:13:27Z

Codecov Report

Merging #4033 (c6b082d) into master (f60ab06) will increase coverage by 0.03%.
The diff coverage is 50.87%.

@@             Coverage Diff              @@
##             master    #4033      +/-   ##
============================================
+ Coverage     73.36%   73.40%   +0.03%     
- Complexity    15893    15947      +54     
============================================
  Files          1385     1388       +3     
  Lines         60755    60883     +128     
  Branches       7701     7714      +13     
============================================
+ Hits          44575    44689     +114     
- Misses        12283    12291       +8     
- Partials       3897     3903       +6

Impacted Files	Coverage Δ
...ria/common/grpc/GsonGrpcJsonMarshallerBuilder.java	`18.18% <18.18%> (ø)`
...rmeria/internal/client/grpc/GrpcClientFactory.java	`84.53% <50.00%> (-3.38%)`	⬇️
...inecorp/armeria/server/grpc/FramedGrpcService.java	`82.06% <63.63%> (-2.33%)`	⬇️
...rp/armeria/common/grpc/GsonGrpcJsonMarshaller.java	`85.71% <85.71%> (ø)`
...necorp/armeria/common/grpc/GrpcJsonMarshaller.java	`100.00% <100.00%> (ø)`
...rmeria/spring/actuate/WebOperationServiceUtil.java	`82.60% <0.00%> (-17.40%)`	⬇️
...ecorp/armeria/server/LengthBasedServiceNaming.java	`75.00% <0.00%> (-16.67%)`	⬇️
.../armeria/client/circuitbreaker/CircuitBreaker.java	`60.00% <0.00%> (-15.00%)`	⬇️
.../linecorp/armeria/client/retrofit2/PipeBuffer.java	`79.06% <0.00%> (-9.31%)`	⬇️
.../linecorp/armeria/server/tomcat/TomcatService.java	`57.95% <0.00%> (-2.45%)`	⬇️
... and 42 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 f60ab06...c6b082d. Read the comment docs.

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcJsonUtil.java

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/UpstreamJsonMarshaller.java

ikhoon · 2022-01-17T12:11:54Z

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshallerBuilder.java

+        } else if (grpcJsonMarshallType == GrpcJsonMarshallType.PROTOBUF_JACKSON) {
+            return GrpcJsonUtil.protobufJacksonJsonMarshaller(serviceDescriptor, jsonMarshallerCustomizer);
+        } else if (grpcJsonMarshallType == GrpcJsonMarshallType.UPSTREAM) {
+            return UpstreamJsonMarshaller.INSTANCE;


GrpcJsonMarshallType.UPSTREAM and jsonMarshallerCustomizer are mutually exclusive.
Originally, this builder is designed for protobuf-jackson.
How about adding some factory methods instead of GrpcJsonMarshallType?

// Choose the JSON codec depending on a proto syntax. GrpcJsonMarshaller.of(); // Always uses protobuf-jackon GrpcJsonMarshaller.ofJackson(...); GrpcJsonMarshaller.builderForJackson() .jsonMarshallerCustomizer(...) .build(ServiceDescriptor); // Always uses protobuf-util's JsonFormat GrpcJsonMarshaller.ofGson(...);

Good point.

I've updated my thoughts on the default implementation in the PR description.

I'm thinking it might be better to leave the default as-is, and just providing a gson implementation which users can opt-in. This can be slightly inconvenient since users need to add compatibility for both client and server-side, but at least there will be no breaking changes.

GrpcService.builder() .jsonMarshallerFactory(sd -> GrpcJsonMarshaller.ofGson()) .build(); ... GrpcClients.builder(server.httpUri(GrpcSerializationFormats.JSON)) .jsonMarshallerFactory(sd -> GrpcJsonMarshaller.ofGson()) .build(Proto2ServiceBlockingStub.class);

I'm thinking it might be better to leave the default as-is

Sounds good. 👍

ikhoon

Nice work, @jrhee17!

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

ikhoon · 2022-01-18T12:58:22Z

grpc/src/main/java/com/linecorp/armeria/common/grpc/GsonGrpcJsonMarshallerBuilder.java

+     * Sets a {@link Consumer} that can customize the {@link JsonFormat.Parser} for {@link Message}
+     * used when handling JSON payloads in the service.


Suggested change

* Sets a {@link Consumer} that can customize the {@link JsonFormat.Parser} for {@link Message}

* used when handling JSON payloads in the service.

* Adds a {@link Consumer} that can customize the {@link JsonFormat.Parser}

* used when deserializing a JSON payload into a {@link Message}.

grpc/src/main/java/com/linecorp/armeria/common/grpc/GsonGrpcJsonMarshallerBuilder.java

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GsonGrpcJsonMarshaller.java

jrhee17 · 2022-01-19T05:22:23Z

Note: also added a warning log

Sample ⬇️

14:17:46.728 [Test worker] WARN  c.l.armeria.common.grpc.GrpcJsonUtil - Failed to instantiate a json marshaller for [MethodDescriptor{fullMethodName=example.grpc.protoversion.Proto2Service/echo, type=UNARY, idempotent=false, safe=false, sampledToLocalTracing=true, requestMarshaller=io.grpc.protobuf.lite.ProtoLiteUtils$MessageMarshaller@4e3f2908, responseMarshaller=io.grpc.protobuf.lite.ProtoLiteUtils$MessageMarshaller@7e87ef9e, schemaDescriptor=example.armeria.grpc.Proto2ServiceGrpc$Proto2ServiceMethodDescriptorSupplier@e3b0369}]. Consider using GrpcJsonMarshaller.ofGson instead.

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonUtil.java

jrhee17 · 2022-01-19T05:37:35Z

Updated Version of the warning log

14:34:41.525 [Test worker] WARN  c.l.armeria.common.grpc.GrpcJsonUtil - Failed to instantiate a json marshaller for [MethodDescriptor{fullMethodName=example.grpc.protoversion.Proto2Service/echo, type=UNARY, idempotent=false, safe=false, sampledToLocalTracing=true, requestMarshaller=io.grpc.protobuf.lite.ProtoLiteUtils$MessageMarshaller@67e0ff3a, responseMarshaller=io.grpc.protobuf.lite.ProtoLiteUtils$MessageMarshaller@7b2bf745, schemaDescriptor=example.armeria.grpc.Proto2ServiceGrpc$Proto2ServiceMethodDescriptorSupplier@6fe595dc}]. Consider using com.linecorp.armeria.common.grpc.GrpcJsonMarshaller.ofGson() instead.
java.lang.IllegalStateException: Could not find setter.
	at org.curioswitch.common.protobuf.json.ProtoFieldInfo.setValueMethod(ProtoFieldInfo.java:230)
	at org.curioswitch.common.protobuf.json.DoParse.setFieldValue(DoParse.java:452)
...

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

anuraaga · 2022-01-19T05:36:46Z

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

        return new GrpcJsonMarshallerBuilder();
    }

+    /**
+     * Returns a newly-created {@link GrpcJsonMarshaller} which serializes and deserializes {@link Message}.


Should add notes that the performance is much lower and should only be used if the default has problems, for example enum values in proto2 files. In practical terms, ofGson is not fast enough for production workloads - actually I don't know if having these factories solves the user's reported issue since it still requires configuration, as opposed to configuring the serialization formats. AFAICT, the user didn't need JSON, but they were having issues because the default serialization formats includes it. Requiring a different option may not actually help them.

Should add notes that the performance is much lower

Thanks for looking though this PR! Left an additional note in the javadocs that addresses this.

AFAICT, the user didn't need JSON, but they were having issues because the default serialization formats includes it.

I still think this PR has meaning in that users who are committed to using armeria still have an option to use the upstream marshaller implementation if they'd like.

Having said this, if we want to avoid breaking changes and avoid user-side extra configuration, I guess the other option we have is to just swallow the exception and warn when building a MessageMarshaller fails. Let me also consider this.

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonUtil.java

jrhee17 · 2022-01-19T09:23:23Z

Note to reviewers: Updated behavior to swallow an exception and warn if constructing jsonMarshallers failed

trustin · 2022-01-21T07:47:35Z

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

+                               .collect(toImmutableMap(ServiceDescriptor::getName, jsonMarshallerFactory));
+            } catch (Exception e) {
+                logger.warn("Failed to instantiate a JSON marshaller. Consider " +
+                            "disabling gRPC-JSON serialization or using {}.ofGson() instead.",


disabling gRPC-JSON serialization with <method reference> or using ...

minwoox

Thanks a lot, @jrhee17!

minwoox · 2022-01-21T08:45:25Z

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

+     * and {@code Gson} to serialize and deserialize messages.
+     * Note:
+     * <a href="https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/util/JsonFormat">JsonFormat</a>
+     * has a known performance issue. Use this method only when


Could you leave a link to the performance issue? 😄

minwoox · 2022-01-21T08:50:05Z

grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcJsonMarshaller.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 LINE Corporation
+ * Copyright 2022 LINE Corporation


jrhee17 · 2022-01-25T01:31:32Z

Sorry about the last minute change, but I realized that client-side could also benefit from richer exception messages.
I'd appreciate if reviewers can take a look at the following commit 58acdbc

ikhoon · 2022-01-25T06:01:49Z

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

+            try {
+                jsonMarshaller = options.get(GrpcClientOptions.GRPC_JSON_MARSHALLER_FACTORY)
+                                        .apply(serviceDescriptor);
+            } catch (RuntimeException e) {


How about catching Exception like FramedGrpcService?

trustin

Nice work, @jrhee17 🙇

- grpc + armeria 사용 시 JsonMarshaller 초기화 실패하는 버그가 있는 듯 함. - line/armeria#4033 - jsonMarshallerFactory 이용해서 JsonMarshaller 초기화

minimal implementation

fa4b834

jrhee17 added breaking change defect labels Jan 16, 2022

jrhee17 added this to the 1.14.0 milestone Jan 16, 2022

jrhee17 removed the breaking change label Jan 17, 2022

jrhee17 added 3 commits January 17, 2022 10:29

add more unit/it tests

858edbb

add more unit tests

965310e

also handle proto2 included in proto3 file case

6e71b03

jrhee17 marked this pull request as ready for review January 17, 2022 07:34

jrhee17 requested review from ikhoon, minwoox and trustin as code owners January 17, 2022 07:34

jrhee17 marked this pull request as draft January 17, 2022 08:09

jrhee17 removed request for trustin, ikhoon and minwoox January 17, 2022 08:10

jrhee17 added 2 commits January 17, 2022 17:15

fix failing test

b708727

add whitespace option by default

2c47eed

ikhoon reviewed Jan 17, 2022

View reviewed changes

jrhee17 force-pushed the bugfix/grpc-json-type branch 2 times, most recently from 5bf6f55 to f24c861 Compare January 18, 2022 05:54

use protobuf-jackson by default, provide an upstream alternative

58c0609

jrhee17 force-pushed the bugfix/grpc-json-type branch from f24c861 to 58c0609 Compare January 18, 2022 07:03

jrhee17 marked this pull request as ready for review January 18, 2022 08:34

jrhee17 requested review from minwoox and trustin January 18, 2022 08:34

ikhoon approved these changes Jan 18, 2022

View reviewed changes

fix javadocs, add warning log

1bd8e83

jrhee17 force-pushed the bugfix/grpc-json-type branch from 4a22f5b to 1bd8e83 Compare January 19, 2022 05:22

ikhoon reviewed Jan 19, 2022

View reviewed changes

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

update the warning log message

9528f02

jrhee17 force-pushed the bugfix/grpc-json-type branch from 5fcac6b to 9528f02 Compare January 19, 2022 05:39

anuraaga reviewed Jan 19, 2022

View reviewed changes

jrhee17 added 2 commits January 19, 2022 16:11

add documentation on performance

e181b84

let users know removing grpc serialization formats is also an option

7be0eaa

trustin reviewed Jan 19, 2022

View reviewed changes

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

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

catch marshall exceptions on startup and warn

a54508e

jrhee17 force-pushed the bugfix/grpc-json-type branch from 3fc0593 to a54508e Compare January 19, 2022 09:15

anuraaga approved these changes Jan 19, 2022

View reviewed changes

jrhee17 changed the title ~~Use upstream json marshall implementation for proto2~~ Introduce GsonGrpcJsonMarshaller, catch jsonMarshaller initialization failure Jan 19, 2022

jrhee17 changed the title ~~Introduce GsonGrpcJsonMarshaller, catch jsonMarshaller initialization failure~~ Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure Jan 19, 2022

trustin reviewed Jan 21, 2022

View reviewed changes

minwoox approved these changes Jan 24, 2022

View reviewed changes

link benchmark, improve exception message, handle client side

58acdbc

ikhoon reviewed Jan 25, 2022

View reviewed changes

catch exception for client side

c6b082d

trustin approved these changes Jan 25, 2022

View reviewed changes

trustin merged commit 8df42aa into line:master Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure #4033

Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure #4033

jrhee17 commented Jan 16, 2022 •

edited

Loading

codecov bot commented Jan 16, 2022 •

edited

Loading

ikhoon Jan 17, 2022

jrhee17 Jan 18, 2022 •

edited

Loading

ikhoon Jan 18, 2022

ikhoon left a comment

ikhoon Jan 18, 2022

jrhee17 commented Jan 19, 2022

jrhee17 commented Jan 19, 2022

anuraaga Jan 19, 2022

jrhee17 Jan 19, 2022 •

edited

Loading

jrhee17 commented Jan 19, 2022 •

edited

Loading

trustin Jan 21, 2022

minwoox left a comment

minwoox Jan 21, 2022

minwoox Jan 21, 2022

jrhee17 commented Jan 25, 2022

ikhoon Jan 25, 2022

trustin left a comment

		* Sets a {@link Consumer} that can customize the {@link JsonFormat.Parser} for {@link Message}
		* used when handling JSON payloads in the service.

Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure #4033

Introduce GsonGrpcJsonMarshaller and catch JsonMarshaller initialization failure #4033

Conversation

jrhee17 commented Jan 16, 2022 • edited Loading

codecov bot commented Jan 16, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jrhee17 Jan 18, 2022 • edited Loading

Choose a reason for hiding this 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

jrhee17 commented Jan 19, 2022

jrhee17 commented Jan 19, 2022

Choose a reason for hiding this comment

jrhee17 Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

jrhee17 commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 commented Jan 25, 2022

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

jrhee17 commented Jan 16, 2022 •

edited

Loading

codecov bot commented Jan 16, 2022 •

edited

Loading

jrhee17 Jan 18, 2022 •

edited

Loading

jrhee17 Jan 19, 2022 •

edited

Loading

jrhee17 commented Jan 19, 2022 •

edited

Loading