Fix Dialogue client memory leak when using generateErrorParameterFormatRespectingDialogueInterfaces

[aldexis]

Before this PR

When you set generateErrorParameterFormatRespectingDialogueInterfaces to true, the DialogueClientsDnsIntegrationTest#nonReloadableClientConfig and corresponding nonReloadableServiceConfig start failing at the last step, which verifies that when you orphan the dialogue client, the corresponding metric gets cleaned up.

This actually does not work when using generateErrorParameterFormatRespectingDialogueInterfaces because it generates code like

private final Deserializer<Void> voidToVoidDeserializer = _runtime.bodySerDe()
        .emptyBodyDeserializer(createExceptionDeserializerArgs(new TypeMarker<Void>() {}));

The new TypeMarker<Void>() {} is the issue here, because

• It keeps a reference to the parent object (i.e. the dialogue client)
• It is retained by the ConjureBodySerde's exceptionDeserializers cache (since it's part of the key, and it doesn't use weak keys)

This means that dialogue clients constructed with generateErrorParameterFormatRespectingDialogueInterfaces cannot get GC'ed, which in general isn't a problem since they're meant to be long-lived, but could actually result in a memory leak when using sticky clients, which tend to create new clients every time

Edit: This is actually unlikely to be a big problem, since two instances of the same client would end up hitting the same cache keys, due to TypeMarker checking equality on the underlying Type, and since we don't use weak keys, so only the first created instance would be incorrectly retained. Still not ideal though, and worth a simple fix.

After this PR

==COMMIT_MSG==
Fix Dialogue client memory leak when using generateErrorParameterFormatRespectingDialogueInterfaces
==COMMIT_MSG==

The fix here is to not use the ExceptionDeserializerArgs objects directly as the cache key, but instead extract its information to build the actual cache key.

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

Type (Select exactly one)

• Feature (Adding new functionality)
• Improvement (Improving existing functionality)
• Fix (Fixing an issue with existing functionality)
• Break (Creating a new major version by breaking public APIs)
• Deprecation (Removing functionality in a non-breaking way)
• Migration (Automatically moving data/functionality to a new system)

Description

Fix Dialogue client memory leak when using generateErrorParameterFormatRespectingDialogueInterfaces

Check the box to generate changelog(s)

• Generate changelog entry

[aldexis]

Couple of things regarding this:

• ExceptionDeserializerArgs was implementing toString, equals and hashCode because it was used as the cache key. This is now unnecessary afaict and could probably be removed, but would be a behavior break.
• If we were to add more elements to ExceptionDeserializerArgs in the future, the cache key wouldn't take them into account by default. I've debated instead adding a ExceptionDeserializerArgs.CacheKey class directly in ExceptionDeserializerArgs, which would make the behavior/requirement more obvious.
• ExceptionDeserializerArgs itself was probably a mistake, and we should instead have
  • built Map<String, ErrorExceptionPair<?, ?>> errorNameToExceptionTypeMarkers directly in the dialogue clients (which we already effectively do, but end up doing once per endpoint, which is inefficient when it's always the same)
  • made deserializer(TypeMarker<T> token, Map<String, ErrorExceptionPair<?, ?>> errorNameToExceptionTypeMarkers) (which would have constructed its key then)

Unfortunately, this is now part of the API and every generated dialogue client, so we can't simply break it. We could possibly still improve future codegen, but palantir/conjure-java#2869 may be better anyway

[pkoenig10]

If we were to add more elements to ExceptionDeserializerArgs in the future, the cache key wouldn't take them into account by default.

This actually seems fine to me. The arguments to deserializer and the key used to cache some values internally may not necessarily be the same.

It's sort of impossible to mess this up. If a value is necessary for constructing the deserializer, then it will have to be made part of the cache key so it's accessible to the deserializer.

ExceptionDeserializerArgs itself was probably a mistake

Not sure I agree with this.

Having a single "request" parameter like ExceptionDeserializerArgs allows us to add additional "parameters" without causing ABI breaks.

[aldexis]

If a value is necessary for constructing the deserializer, then it will have to be made part of the cache key so it's accessible to the deserializer.

Ah yeah, that makes sense 👍

Having a single "request" parameter like ExceptionDeserializerArgs allows us to add additional "parameters" without causing ABI breaks.

That's a fair point, but we still can only add new optional fields regardless, which could also have been done by adding method overloads instead. Not saying overloads are ideal either, and I agree there are some nice aspects to ExceptionDeserializerArgs. Ultimately, the problem is more so about bad usage of that class (duplicating map creation, as well as using it as cache keys), whereas it's more effective as a pure argument-passing class (just wrapping other objects that are created separately, and shared where sensible), and shouldn't live beyond that

[changelog-app]

✅ Successfully generated changelog entry!

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

---

📋Changelog Preview

🐛 Fixes

• Fix Dialogue client memory leak when using generateErrorParameterFormatRespectingDialogueInterfaces (#3024)

[pkoenig10]

What's the purpose of this? Just to exercise this code path?

[aldexis]

Yes - it doesn't get exercised otherwise.

[aldexis]

This is also the desired default as of a few months ago, and thus it makes sense to update the tests to test against it

[autorelease3]

Released 6.30.0

This creates a tag and GitHub release. Additional build or deployment steps may still be in progress.