Use configured gson instance for toString by henryju · Pull Request #772 · eclipse-lsp4j/lsp4j

henryju · 2023-11-07T18:41:30Z

This is important when users have registered custom type adapters

Fixes #768

pisv

Overall, the patch looks reasonable to me.

There are a few stylistic nitpicks.

However, my main concern is the removal of toString() methods from Message and related classes.

This can create not only inconveniences while debugging, but also problems with logging.

Even in TracingMessageConsumer itself, there are a number of places where messages are logged, which need to be changed in the same way as https://github.com/henryju/lsp4j/blob/bd47976a6a45957e5be9973e74196e1ce5e3f212/org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/TracingMessageConsumer.java#L176.

Basically, each of the log calls in LSP4J needs to be analyzed, and amended if necessary. What's more, we'll need to ensure every time, both now and in the future, that there is an accessible instance of MessageJsonHandler in every such case. What's even more, the existing clients that log messages using Message.toString() would also be affected and would need to be changed in the same way as LSP4J itself.

I don't see a good solution here, but it seems that we need to restore those 'basic' toString() methods, make sure they no longer throw exceptions, and try to use the new MessageJsonHandler.toString(Object) method for message logging where it is possible (as, for example, in TracingMessageConsumer itself).

WDYT?

pisv · 2023-11-08T12:59:18Z

 			RequestMetadata requestMetadata = sentRequests.remove(id);
 			if (requestMetadata == null) {
-				LOG.log(WARNING, String.format("Unmatched response message: %s", message));
+				LOG.log(WARNING, String.format("Unmatched response message: %s", jsonHandler.toString(message)));


The same change needs to be done in all other places where messages are logged, potentially not only in this file. This is my main concern.

I fixed the one I could find, but I agree there is a risk of someone still relying on the default toString of a message. I will try to think about a better solution.

pisv · 2023-11-08T14:47:39Z

To illustrate, here is how the console log looks like after running the IntegrationTest with the patch in its current form:

Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageSending
WARNING: Unmatched response message: org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage@5e1f1cb
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageSending
WARNING: Unmatched response message: org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage@a0b2606
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageReceiving
WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "BAR"
  }
}
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageReceiving
WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "FOO",
    "customAdapter": "/Users/vladimir/git/lsp4j/org.eclipse.lsp4j.jsonrpc"
  }
}

henryju · 2023-11-09T14:00:07Z

I fixed most of the feedback, except the point about having no more toString on the various Message classes. I will have another look a bit later, to see if that would be possible to inject the jsonHandler into each of those objects when they are created.

pisv

Thank you very much for your work @henryju. I have left additional comments.

pisv · 2023-11-09T14:38:27Z

 			Map<String, RequestMetadata> sentRequests,
 			Map<String, RequestMetadata> receivedRequests,
 			PrintWriter printWriter,
+			MessageJsonHandler jsonHandler,


This is a breaking API change: a new parameter is added to the existing constructors. Also, when the constructor is called from the MessageTracer, the jsonHandler argument can now be null. I'd suggest either to add a new constructor with the additional jsonHandler parameter, or to add a setter like in MessageTracer. In any case, if jsonHandler is null, the class needs to fallback to the old behaviour.

By

class needs to fallback to the old behaviour.

do you mean it should create a new Gson instance and try to serialize it without custom adapters? I find this dangerous as it may still throw an exception as before.

I think that in general we need to keep the old behavior as a fallback in cases where a MessageJsonHandler is unavailable for whatever reason, but make sure that possible exceptions are properly handled and no longer thrown from those toString methods. The goal is to avoid any breaking changes in API or behavior.

To illustrate, WebSocketLauncherBuilders are currently broken, because they completely override the Builder.create() method and we have not applied the messageTracer.setJsonHandler(jsonHandler) change there. But such cases may also happen in existing clients. If there were a fallback strategy in TracingMessageConsumer, they would not have been broken.

Similarly, even if it would be possible to inject a MessageJsonHandler into each of the Message objects when they are created inside the framework as you have suggested, we still need to have a 'baseline' behaviour in the Message.toString method to avoid breaking existing clients that may create them outside the framework.

I made another attempt. I didn't like the fact that the jsonHandler could be null on the MessageTracer, so I went for a deprecation approach.

If you ask me, I liked the previous approach more. To me, the problem looks like a 'progressive enhancement' of the old behaviour in cases where a MessageJsonHandler is available. As I have mentioned, there might still be cases where it will not be available, so I can see no reason why we should require it in a constructor. This is my 'informed opinion' so to speak. Nevertheless, I really appreciate your effort.

Since MessageTracer constructor is package private, I take for granted that it is not instantiated/extended outside lsp4j. So changing it is fine.
Regarding TracingMessageConsumer, I deprecated the old constructors, but keep them working as before using a fresh MessageJsonHandler instance.
If you think the two use cases are valid on the long run, I can remove deprecation tags.
The last "issue" is the removal of toString on the various messages. I don't like reverting to the old behavior, since this could be really creating bugs on user side. What about providing toString with the best information we could, but not relying on gson serialization?

pisv · 2023-11-09T18:35:31Z

 	private final Map<String, RequestMetadata> receivedRequests = new HashMap<>();

-	MessageTracer(PrintWriter printWriter) {
+	MessageTracer(PrintWriter printWriter, MessageJsonHandler jsonHandler) {


This is also a breaking API change.

The constructor is package private, so I would say this is not a breaking API change

My bad. You are right, of course. But I'd still prefer a setter in this case.

pisv · 2023-11-10T09:38:18Z

To sum up and hopefully clarify, here is what I'd like to see in this PR.

Thanks to the prompt by @szarnekow and the following changes by @henryju, I have been persuaded that there is a clean non-breaking solution to this problem.

So, one of the main goals should be to preserve all of the existing API elements and augment the existing behaviour rather than trying to replace it.

To put it in a perspective, there were no issues reported about this problem since 2019 when the TracingMessageConsumer was introduced. This seems to indicate that for most of the LSP4J clients the current implementation works just fine.

Therefore, let's keep the current API and implementation to avoid breaking any of the existing clients, and just augment it with the enhanced implementation where it is possible, using the old one as a fallback.

To that end, let's first reinstate all of the removed methods and their behaviour, including the static MessageJsonHandler.toString method, Message.toString, CancelParams.toString, ResponseError.toString.

Let's rename the (new) instance MessageJsonHandler.toString method to e.g. format.

Let's use a setter for passing a MessageJsonHandler to both MessageTracer and TracingMessageConsumer as it is the least invasive approach and works just fine in this case.

When no MessageJsonHandler is set on a TracingMessageConsumer, let's fallback to the existing behaviour.

That would be a clean non-breaking solution I'd like to see.

As a bonus, let's make sure that possible exceptions are properly handled in the existing static MessageJsonHandler.toString method, so that it becomes more resilient if some of the required type adapters are missing.

However, I'd consider this bonus part as optional for this PR.

henryju · 2023-11-10T12:55:53Z

I won't say I am super convinced by keeping dangerous toString implementations, but I will respect your decision. I have some other things to do today, so I will come back to this next week.

pisv · 2023-11-10T14:04:45Z

I won't say I am super convinced by keeping dangerous toString implementations

On one hand, at least it will not make things any worse for existing clients.

With your current proposal, clients might start to see something like

WARNING: Unmatched response message: Message{id=1}

instead of

WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "FOO"
  }
}

I don't think it would be fine for most purposes.

On the other hand, we can try to make the 'dangerous' toString implementations less dangerous:

As a bonus, let's make sure that possible exceptions are properly handled in the existing static MessageJsonHandler.toString method, so that it becomes more resilient if some of the required type adapters are missing.

pisv · 2023-11-10T14:38:03Z


 		public Builder<T> traceMessages(PrintWriter tracer) {
 			if (tracer != null) {
-				this.messageTracer = new MessageTracer(tracer);


With your current approach, we'd still need to keep this line with the one-arg constructor as a fallback. Otherwise, an existing subclass that completely overrides Builder.create() would have no messageTracer set, which is a breaking change. Similarly, we'd need to make the new two-arg constructor public so that such subclasses could reset the default messageTracer in their create() implementations. That's why I much prefer your previous approach with MessageTracer.setJsonHandler. It is so much less of a hassle.

pisv · 2023-11-10T17:30:31Z

For the sake of convenience, I have pushed what I'd consider a clean patch against main that reflects the points I have depicted above. (It doesn't include the bonus part.)

If you don't mind, let's use it as a baseline for further discussion and development.

pisv · 2023-11-15T14:47:58Z

Let's see if I can come up with something along the lines sketched out in #768 (comment) to make the 'dangerous' toString methods less dangerous...

pisv · 2023-11-16T13:07:53Z

Note that I had to copy ToStringBuilder from the org.eclipse.lsp4j.util package as it could not be accessed from here. I also had to add a couple of new methods: addAllFields(Predicate<Field>), which is called from Message.toStringFallback(), and, for the sake of symmetry, addDeclaredFields(Predicate<Field>).

I don't think it is good for us to have three copies of ToStringBuilder in LSP4J now.

It seems that the best solution would be to extract a separate bundle such as org.eclipse.lsp4j.common and move ToStringBuilder there, perhaps with some other common stuff, so that it could be accessed from everywhere.

However, I'd consider that to be out of the scope of this PR.

In the meantime, I think we'll have to live with yet another copy of ToStringBuilder here as a package-private class. At least, it is not API and can be safely removed at any time.

As lsp4j.jsonrpc is at the root of all dependency chains I think having it just in this new place is fine. And it is certainly fine to defer to a future PR

I'll leave it to a future PR to keep this one free of the associated noise.

pisv · 2023-11-16T13:40:02Z

 */
 public abstract class Message {

+	private transient MessageJsonHandler jsonHandler;


I'm not entirely comfortable with this idea of providing a way to inject a MessageJsonHandler into a Message and storing it in a transient field, as it can potentially break existing reflective code that enumerates/introspects message properties using fields rather than JavaBeans-properties.

Note that I had to rename Message.getJsonHandler() to jsonHandler() previously so that ReflectiveMessageValidator did not actually fail by treating jsonHandler as a general property of the message, which seems to indicate that it might be more than a hypothetical issue.

I am not really concerned by this - but I have added an entry to the Changelog that mentions what to do in this case.

pisv · 2023-11-16T13:58:56Z

 		Message message = gson.fromJson(jsonReader, Message.class);

 		if (message != null) {
+			message.setJsonHandler(this);


This call might seem unnecessary, since we already set the MessageJsonHandler in MessageTypeAdapter and DebugMessageTypeAdapter as soon as a message is created, but it serves those clients that register their own message type adapter (similarly to how DebugMessageTypeAdapter is registered), but don't set the MessageJsonHandler for created messages.

pisv · 2023-11-16T14:16:19Z

 	private final MessageConsumer out;
 	private final Endpoint localEndpoint;
 	private final Function<Throwable, ResponseError> exceptionHandler;
+	private MessageJsonHandler jsonHandler;


Note that RemoteEndpoint now has the reference to a MessageJsonHandler to be able to inject the handler into the messages it creates.

pisv · 2023-11-16T18:32:14Z

@jonahgraham Could you please review this PR with a fresh pair of eyes?

We tried to enhance the existing implementation without any breaking changes by passing a specific MessageJsonHandler to the TracingMessageConsumer and Message so that the static MessageJsonHandler.toString(Object) method is called only as a fallback now.

If all else fails, Message.toString(), CancelParams.toString(), and ResponseError.toString() will fallback to an alternative implementation based on a ToStringBuilder.

As far as I can see, the main potential issue with the current patch is that existing reflective code might be broken by the addition of the Message.jsonHandler field. (There is a more detailed comment above about this issue.) Apart from that, it should cause no issues for existing clients.

pisv · 2024-01-11T15:19:04Z

Regarding the potential issue with existing reflective code, I now think that it would probably be sufficient to just mention it in the CHANGELOG as a potential breaking change, but I would still appreciate an extra pair of eyes to review this PR.

It would be great if we could merge it before the 0.22.0 release.

jonahgraham · 2024-01-11T16:00:48Z

I added it to the 0.22.0 milestone. It fell off the bottom of my todo list, I will schedule to look at it sometime before the 0.22.0 release. See conversation about timing in #732

pisv · 2024-01-11T16:41:31Z

I added it to the 0.22.0 milestone. It fell off the bottom of my todo list, I will schedule to look at it sometime before the 0.22.0 release. See conversation about timing in #732

OK. Thank you!

This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768

jonahgraham

This looks fine to me. I am about to push a rebased squashed version with the above mentioned changelog entry too to ensure that it builds cleanly.

jonahgraham · 2024-02-08T21:01:05Z

I'll merge this before I build 0.22.0 next ~~year~~ week unless I hear concerns back.

@henryju and @pisv thank you for your efforts here.

jonahgraham · 2024-02-09T01:01:53Z

next year

next *week :-)

pisv

@jonahgraham Thank you very much for your review and approval.

May I suggest a couple of minor enhancements to the changelog entry?

Co-authored-by: Vladimir Piskarev <pisv@1c.ru>

jonahgraham · 2024-02-09T20:12:00Z

Thanks everyone. This is now committed and the next release of LSP4J (expected next week) will contain this improvement.

pisv reviewed Nov 8, 2023

View reviewed changes

szarnekow reviewed Nov 8, 2023

View reviewed changes

Comment thread org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/Launcher.java Outdated

szarnekow reviewed Nov 8, 2023

View reviewed changes

Comment thread org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/MessageJsonHandler.java

henryju force-pushed the fix/toString_custom_gson_adapter branch from bd47976 to 016ccdc Compare November 9, 2023 13:42

pisv reviewed Nov 9, 2023

View reviewed changes

henryju force-pushed the fix/toString_custom_gson_adapter branch from 8d89633 to df463f8 Compare November 9, 2023 17:39

pisv reviewed Nov 9, 2023

View reviewed changes

henryju force-pushed the fix/toString_custom_gson_adapter branch 2 times, most recently from 05b6708 to 6cd0673 Compare November 10, 2023 07:39

pisv reviewed Nov 10, 2023

View reviewed changes

pisv mentioned this pull request Nov 12, 2023

MessageJsonHandler::toString doesn't work with custom type adapters #768

Closed

pisv reviewed Nov 16, 2023

View reviewed changes

jonahgraham added this to the 0.22.0 milestone Jan 11, 2024

Use configured gson instance for toString

03a4a14

This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768

jonahgraham approved these changes Feb 8, 2024

View reviewed changes

jonahgraham force-pushed the fix/toString_custom_gson_adapter branch from e8ab10c to 03a4a14 Compare February 8, 2024 20:55

jonahgraham requested a review from pisv February 8, 2024 21:00

pisv reviewed Feb 9, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread CHANGELOG.md Outdated

Apply suggestions from code review

b12d96d

Co-authored-by: Vladimir Piskarev <pisv@1c.ru>

jonahgraham merged commit db27a95 into eclipse-lsp4j:main Feb 9, 2024

henryju deleted the fix/toString_custom_gson_adapter branch February 12, 2024 12:44

jonahgraham mentioned this pull request Feb 29, 2024

OOM when working with large files because of huge DocumentSymbols response #815

Open

pisv mentioned this pull request Feb 3, 2026

TracingMessageConsumer uses static MessageJsonHandler, leading to Gson errors #947

Closed

Conversation

henryju commented Nov 7, 2023

Uh oh!

pisv left a comment • 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!

Uh oh!

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!

Uh oh!

pisv commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henryju commented Nov 9, 2023

Uh oh!

pisv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pisv Nov 9, 2023 • 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!

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!

pisv Nov 9, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pisv Nov 10, 2023 • 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!

pisv commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryju commented Nov 10, 2023

Uh oh!

pisv commented Nov 10, 2023 • 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!

pisv commented Nov 10, 2023

Uh oh!

pisv commented Nov 15, 2023

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!

pisv left a comment •

edited

Loading

pisv commented Nov 8, 2023 •

edited

Loading

pisv Nov 9, 2023 •

edited

Loading

pisv Nov 9, 2023 •

edited

Loading

pisv Nov 10, 2023 •

edited

Loading

pisv commented Nov 10, 2023 •

edited

Loading

pisv commented Nov 10, 2023 •

edited

Loading

jonahgraham commented Feb 8, 2024 •

edited

Loading