Dispoing JsonDocument

[JeffAshton]

Disposing JsonDocument as per documentation:

This class utilizes resources from pooled memory to minimize the impact of the garbage collector (GC) in high-usage scenarios. Failure to properly dispose this object will result in the memory not being returned to the pool, which will increase GC impact across various parts of the framework.

https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsondocument?view=net-5.0

[JeffAshton]

@eli-darkly Looks like reviewers aren't automatically assigned

[eli-darkly]

@JeffAshton No need to assign reviewers or @-mention anyone— the team is alerted to every GitHub issue and PR. Just wasn't able to look at this on Friday, sorry. Looking now.

[eli-darkly]

Looks good - thanks for catching this. We should be able to put out a patch release of this and of the SDK within the next couple days.

[eli-darkly]

@JeffAshton How high a priority is this patch for you? The reason I ask is that we are currently waiting on a new code-signing certificate which we'd like to transition to before the old one expires, and it might be simplest to postpone the next release a few days so we don't have to immediately do an extra release just to use the new cert.

I feel like the scenario addressed in this PR is not a common one— it would only be hit in applications that are using the System.Text.Json API to deserialize a type like User, which is probably rare and is not something that was even supported prior to 6.0.0— so although I agree that it's best to dispose of the object promptly, I'm not sure it's likely to be a major contributor to GC pressure. But if this is something you really are seeing performance impact from, I can get the patch out sooner.

[JeffAshton]

@JeffAshton How high a priority is this patch for you? The reason I ask is that we are currently waiting on a new code-signing certificate which we'd like to transition to before the old one expires, and it might be simplest to postpone the next release a few days so we don't have to immediately do an extra release just to use the new cert.

I feel like the scenario addressed in this PR is not a common one— it would only be hit in applications that are using the System.Text.Json API to deserialize a type like User, which is probably rare and is not something that was even supported prior to 6.0.0— so although I agree that it's best to dispose of the object promptly, I'm not sure it's likely to be a major contributor to GC pressure. But if this is something you really are seeing performance impact from, I can get the patch out sooner.

Being preemptive here. I've seen performance benefits in other applications from disposing. It was non obvious to me when I started using JsonDocument.Parse that the type would be disposable. So I doubled checked when I saw that the library was using System.Text.Json. I'm happy to wait until your new code-signing certificate is ready to go.

[eli-darkly]

Yeah, to be clear, even though the SDK does use System.Text.Json internally on some platforms, this code path doesn't apply to those uses in general— it is specifically for the case where the application chooses to use the reflection-based System.Text.Json APIs to deserialize a LaunchDarkly type, or a class/struct that contains a LaunchDarkly type. The SDK itself doesn't use JsonStreamConverterSystemTextJson.

[eli-darkly]

@JeffAshton The new code-signing certificate is taking longer than expected so I'm going to proceed with the planned .NET SDK release now.