feat: POC of an approach to adding async #78
Conversation
jackmott
changed the title
jackmott
changed the title
This reverts commit 9a259d3.
pkgs/sdk/server/src/Internal/DataStores/PersistentStoreWrapper.cs
Outdated
Show resolved
Hide resolved
| #if NET31_OR_GREATER | ||
| return ValueTask.FromResult<ItemDescriptor?>(item); | ||
| #else | ||
| return new ValueTask<ItemDescriptor?>(Task.FromResult<ItemDescriptor?>(item)); | ||
| #endif |
There was a problem hiding this comment.
| #if NET31_OR_GREATER | |
| return ValueTask.FromResult<ItemDescriptor?>(item); | |
| #else | |
| return new ValueTask<ItemDescriptor?>(Task.FromResult<ItemDescriptor?>(item)); | |
| #endif | |
| return new ValueTask<ItemDescriptor?>(item); |
There was a problem hiding this comment.
Add conditional reference to https://www.nuget.org/packages/System.Threading.Tasks.Extensions for net462?
| @@ -30,7 +31,7 @@ private async Task ArbitraryTask() | |||
| await Task.Delay(TimeSpan.FromTicks(1)); | |||
| } | |||
|
|
|||
| public async Task<SerializedItemDescriptor?> GetAsync(DataKind kind, string key) | |||
| public new async ValueTask<SerializedItemDescriptor?> GetAsync(DataKind kind, string key,CancellationToken cancellationToken = default) | |||
There was a problem hiding this comment.
Haven't pulled the project, but I can't find why you'd need new here?
| return new ValueTask<SerializedItemDescriptor?>(Task.FromResult<SerializedItemDescriptor?>(new SerializedItemDescriptor(0, false, item.SerializedItem))); | ||
| } | ||
| return new ValueTask<SerializedItemDescriptor?>(Task.FromResult<SerializedItemDescriptor?>(item)); |
There was a problem hiding this comment.
Doesn't really matter for tests, but
| return new ValueTask<SerializedItemDescriptor?>(Task.FromResult<SerializedItemDescriptor?>(new SerializedItemDescriptor(0, false, item.SerializedItem))); | |
| } | |
| return new ValueTask<SerializedItemDescriptor?>(Task.FromResult<SerializedItemDescriptor?>(item)); | |
| return new ValueTask<SerializedItemDescriptor?>(new SerializedItemDescriptor(0, false, item.SerializedItem)); | |
| } | |
| return new ValueTask<SerializedItemDescriptor?>(item); |
| return _taskFactory.StartNew(taskFn) | ||
| .GetAwaiter() | ||
| .GetResult() | ||
| .GetAwaiter() | ||
| .GetResult(); |
There was a problem hiding this comment.
Since we're trying to avoid allocations, maybe something like...
| return _taskFactory.StartNew(taskFn) | |
| .GetAwaiter() | |
| .GetResult() | |
| .GetAwaiter() | |
| .GetResult(); | |
| var vt = taskFn(); | |
| return vt.IsCompleted ? vt.Result : WaitSafely(() => vt.AsTask()); |
| var ret = _itemCache is null ? await GetAndDeserializeItemAsync(kind, key, cancellationToken).ConfigureAwait(false) : | ||
| _itemCache.Get(new CacheKey(kind, key)); |
There was a problem hiding this comment.
So I was going to say that since the _itemCache branch here is all sync there would probably be a perf benefit to splitting into paths with and without async/await.
But that made me realize that we don't want an all sync path. The cache loader is async over sync, so we haven't actually avoided any blocking unless we disable the cache. 😭 We'd need to update LaunchDarkly.Cache with IAsyncCache/IAsyncSingleValueCache and wire it through.
|
Hello @jackmott and @dahlbyk, thank you so much for the contribution. While the SDK team cannot get to this immediately, we will look at this contribution, as we want to make the SDK fit the platform's use cases. To understand the platform better, is async usage more common in .NET server applications these days? Is there a concern that the persistent store data read slows down the SDK's performance? Filed Internally as SDK-1096 |
|
Hello @louis-launchdarkly ! Yes async is now very much the default approach for .NET server application. For some reading on this you can refer to: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/best-practices?view=aspnetcore-9.0#avoid-blocking-calls While we understand the past position of LaunchDarkly is that caching makes blocking rare enough to mitigate this issue, it would still be better for this to be a fully async path, and we believe ValueTask in particular can make for an efficient async option here. We have a lot of expertise in house at Olo making our own legacy code async and so we may be able to help work through any tricky issues. |
|
Would be great to have async support! Launch darkly team - Please add it. |
Requirements
Related issues
Describe the solution you've provided
A proof of concept for adding async to the client, if LD is happy with this approach I can complete it and add test coverage. The intent here it to be backwards compatible and follow .NET conventions by adding Async versions of the calls.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.