OctopusDeploy / Halibut

| Public | A secure communication stack for .NET using JSON-RPC over SSL.
Other
12 stars 44 forks source link

Remove the usage of AsyncLocal for DataStream capture in Async Halibut #479

Closed nathanwoctopusdeploy closed 1 year ago

nathanwoctopusdeploy commented 1 year ago

Background

This PR removes the usage of AsyncLocal for DataStream capture in Async Halibut.

while performance testing Server with Async Halibut DataStreams were being retained unexpectedly, although not confirmed, it seems like it may be from the usage of AsyncLocal. Either way, the usage of AsyncLocal for DataStream capture is not required and can be removed and simplified.

Related to https://github.com/OctopusDeploy/Issues/issues/8266

Before

AsyncLocal was used as a store by the JsonSerializer to capture the serialized and deserialized data streams. These would then be referenced in code via the AsyncLocal later on in the ReadMessageAsync and WriteMessageAsync call.

After

AsyncLocal is not used by Async Halibut to capture the streams. Instead, the StreamingContext on the HalibutContractResolver is used as an extension point which gets the DataStreams added to it.

This is then used later to access the streams.

While it is still a bit of an obscure code flow to follow as it has to hook into the serialization and deserialisation logic, it removes the usage of AsyncLocal.

How to review this PR

Quality :heavy_check_mark:

Pre-requisites