C#: [WIP] CSharp Client #3354

X39 · 2025-03-10T23:02:10Z

About

This PR introduces a substantial refactoring of the C# client, reshaping its structure and approach to better align with long-term maintainability and usability. Key changes include:

A clearer folder structure, introducing a dedicated rust folder rather than mixing Rust and C# within a single lib directory.
Adoption of Aspire for testing (leveraging Docker for test environments).
A foundational "base client" targeting .NET Standard.
Introduction of specialized value structures for output values, improving support for non-string scenarios.
A new connection-string format.
A dedicated Hosting library with Aspire support.
Various other improvements aimed at streamlining development and integration.

Context

I want to acknowledge that this PR is quite far-reaching and significantly alters the existing implementation. It initially started as a small proof of concept intended for a Reddit post but quickly evolved into a broader redesign. Given its scope, I’d like to open the discussion for feedback—on both technical direction and overall approach—to ensure alignment with the goals of the C# contributors.

Remaining Work

Key Considerations for Merge

Ensure integration with the existing infrastructure:
- Adapt the benchmarking setup to work with the benchmarks section at root.
- Update the csharp.yml GitHub Action workflow to accommodate Aspire-based testing, which may require adjustments to the test fixture.

Added the implementation of CSharp Native Client in conjunction with the Rust library. New implementation is, as of now, capable to handle client creation and command invocation. Also introduced logging at various levels to help with debugging.

As of now, a few still remain unimplemented. Those are, albeit required, not implemented because i do not know what theese actually express, making FFI implementation complicated. So i need to educate myself on theese first

Introduces a new blocking `SendCommand` method in both the C# and Rust layers, with associated tests and FFI wiring.

# Conflicts: # csharp/lib/BaseClient.cs # csharp/lib/Internals/Message.cs # csharp/lib/glide.csproj # csharp/rust/src/lib.rs # csharp/tests/Integration/TestConfiguration.cs # csharp/tests/tests.csproj

Moved all files of `Valkey.Glide.AppHost` from the root to the `sources` folder for better project organization. Updated references in the solution and unit test project to reflect the new path structure. No functional changes were made to the application.

Refactored connection interface to utilize the new `ConnectionRequest` model, introducing enhanced configurability for TLS, periodic checks, retries, and more. Added multiple structs and enums under `InterOp` namespace for detailed request handling.

…project files

Introduced `ConnectionConfigBuilder` and its associated extensions to simplify and enhance the process of configuring `ConnectionRequest` instances.

Introduced the `Valkey.Glide.Hosting` library for managing connections using a connection string. This includes parsing logic, service collection extensions for DI, and comprehensive unit tests to ensure implementation correctness. Updated documentation and project files to integrate the new library.

Implemented functionality for executing GET and SET commands with extension methods in the Glide client. Added unit tests to ensure correct behavior for both existing and non-existent keys, as well as simple set operations.

Introduced AspireSample.AppHost, AspireSample.Worker, and AspireSample.ServiceDefaults projects to demonstrate distributed application patterns. These include configurations for OpenTelemetry, resilience, health checks, and service discovery. Updated solution file to include the newly added projects.

This updates the Glide library with a comprehensive suite of Redis set commands, supporting various combinations like conditional and expiration-based operations. It also introduces specialized exceptions (e.g., `GlideSetCommandFailedException`) for improved error handling and debugging. Unit testing dependencies and helper methods are updated to reflect these enhancements. A todo was added to add tests for every call against a real valkey instance.

nihohit · 2025-03-11T12:32:49Z

(I'm not a maintainer)
Hi, can you please post before/after benchmarks, to show how this rewrite affects perf?

Sa1Gur · 2025-03-11T12:48:07Z

csharp/.editorconfig

+
+# ReSharper properties
+resharper_align_multiline_binary_patterns = true
+resharper_align_multiline_calls_chain = true


Could you elaborate what is the goal of this change? I mean moving to ReSharper properties. Also consider having separate PR for this, please

As mentioned, this started as a small PoC, and these are essentially my configuration files. I haven’t changed them back yet. I can do so if needed.

As an Open Source project, we should not take dependencies on proprietary tools like resharper. We should assume that many of our developers will not have licenses for this tool. Additionally, there are many CSharp and DotNet settings which we should not be eliminating without a deeper discussion.

Rider is no longer paid but just as free as visual studio community (for FOSS, license differences still exist)

Sa1Gur · 2025-03-11T12:49:49Z

csharp/.gitignore


 # MSTest test Results
 [Tt]est[Rr]esult*/
 [Bb]uild[Ll]og.*

+# NUnit


Could you elaborate on this changes also? Also consider having separate PR for this, please

As mentioned, this started as a small PoC. I achieved this by completely deleting the old folder and replacing everything, as seen in fb1afe5.

The .gitignore is a merge of these default ones:

Microsoft .NET default

GitHub’s JetBrains global ignore

Let me know if any adjustments are needed or if i should roll back :)

Sa1Gur · 2025-03-11T12:53:38Z

csharp/benchmarks/Valkey.Glide.Benchmark/Valkey.Glide.Benchmark.csproj

+
+    <ItemGroup>
+        <PackageReference Include="Aspire.Hosting.Testing" Version="9.1.0" />
+      <PackageReference Include="BenchmarkDotNet" Version="0.14.0" />


Using BenchmarkDotNet is nice touch. Do you see how to make it in line with other benchmark testing for different packages? I would say it is better to keep them in line

There’s definitely a way to make it align. However, I haven’t been able to run the full repository benchmarks yet. That said, I also haven’t spent much time on that part so far.

Sa1Gur · 2025-03-11T12:56:54Z

csharp/benchmarks/Valkey.Glide.Benchmark/ValkeyAspireFixture.cs

+
+    public async Task InitializeAsync()
+    {
+        var appHost = await DistributedApplicationTestingBuilder.CreateAsync<Projects.Valkey_Glide_AppHost>();


If i remember correctly var wasn't recommended by current project code style. Why do we think it is required code style change?

As mentioned, this started as a small PoC, and these are essentially my configuration files; I haven’t changed them back yet. I can do so if needed.

That said, I’d like to point out that this approach aligns better with Rust’s typical style, where variables are mostly declared as let variable = value rather than let variable: Type = value. This follows modern coding practices by leveraging automatic type inference.

While using idiomatic coding patterns is desired, we are also trying to keep various language wrappers aligned to the degree possible. We have maintainers who have depth in one or more of the client languages and following different coding standards for each language makes the transition and understanding of each client more difficult. While type inference is good for experienced developers, it can complicate understanding the code for developers who might not be familiar with a specific language.

Sa1Gur · 2025-03-11T12:57:46Z

csharp/benchmarks/Valkey.Glide.Benchmark/ValkeyAspireFixture.cs

+                WaitBehavior.StopOnResourceUnavailable
+            );
+            if (resourceEvent.Snapshot.HealthStatus is not HealthStatus.Healthy)
+                throw new Exception("Cache is not healthy, aspire initialization failed.");


Let's throw something more concrete

I'm open to suggestions! ^^

I just realized that I mistakenly used the resource name instead of the container type + name. Rephrasing it to something like: "Aspire initialization failed for Valkey Container: 'cache' is not Healthy." seems like a good improvement.

Sa1Gur · 2025-03-11T13:00:48Z

csharp/sources/Valkey.Glide.InterOp/Native/BlockingCommandResult.cs

+
+namespace Valkey.Glide.InterOp.Native;
+
+[SuppressMessage("ReSharper", "InconsistentNaming")]


What is the benefit of having ReSharper specific suppressers?

You're right; i missed the justification.

Rider (or ReSharper) flags this because the naming matches exactly and doesn’t follow the default project-style naming. However, this is intentional since these are FFI structures, which should be kept closely aligned for consistency.

Sa1Gur · 2025-03-11T13:02:10Z

csharp/tests/Valkey.Glide.UnitTests/Valkey.Glide.UnitTests.csproj

+          <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+        </PackageReference>
+        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.13.0" />
+        <PackageReference Include="NSubstitute" Version="5.3.0" />


What is the benefit of using NSubstitute?

It’s a mocking framework, primarily used to easily mock IGlideClient for now, making syntax validation more straightforward.

The key benefit is that it eliminates the need to manually create fully functional mocking classes.

Sa1Gur · 2025-03-11T13:05:43Z

I would say it would be easier to review if we would have different PR for different changes.
Number of new dependencies was introduced - each one separate change
Folder structure changed - another
Code style change - also separate change

Review 30% and will take a look on the rest later.

X39 · 2025-03-11T14:12:08Z

#3354 (comment)

(I'm not a maintainer) Hi, can you please post before/after benchmarks, to show how this rewrite affects perf?

I'm having trouble running the benchmark script due to my setup - I’m on Windows without Go, Java, or Python installed. It’s on my to-do list, but I first need a way to run the benchmark in a container or possibly a VM.

#3354 (comment)

I would say it would be easier to review if we would have different PR for different changes. Number of new dependencies was introduced - each one separate change Folder structure changed - another Code style change - also separate change

Review 30% and will take a look on the rest later.

The reason everything is in one PR is that it originally started as a Proof of Concept. I can try to split it up, but I won’t be able to preserve the commit messages since the PR initially came from a quick "here's how I’d do it" Reddit comment blanking the folder first.

nihohit · 2025-03-11T14:16:16Z

The benchmark script has flags to determine which language to run, so you don't need go or Java. I think you'll only need javascript, to run some of the preparation scripts

jbrinkman · 2025-03-11T15:59:32Z

csharp/.editorconfig

+
+# ReSharper properties
+resharper_align_multiline_binary_patterns = true
+resharper_align_multiline_calls_chain = true


As an Open Source project, we should not take dependencies on proprietary tools like resharper. We should assume that many of our developers will not have licenses for this tool. Additionally, there are many CSharp and DotNet settings which we should not be eliminating without a deeper discussion.

jbrinkman · 2025-03-11T16:55:24Z

.github/workflows/csharp.yml

@@ -88,10 +88,24 @@ jobs:
        steps:
            - uses: actions/checkout@v4

-            - name: Set up dotnet ${{ matrix.dotnet }}
+            - name: Install Node on self-hosted runner


Not sure I understand the need for Node in this pipeline. Can you elaborate on the need?

Copied this over from a different PR (#3323),

csharp/sources/Valkey.Glide.InterOp/NativeClient.cs

+            {
+                Marshal.WriteByte(ptr, 0); // Is allocated on heap
+                for (var i = 0; i < utf8.Length; i++)
+                    Marshal.WriteByte(ptr + i + 1, utf8Ptr[i]);


csharp/sources/Valkey.Glide.InterOp/NativeClient.cs

+            {
+                Marshal.WriteByte(ptr, 1); // Is allocated on buffer (potentially stack)
+                for (var i = 0; i < utf8.Length; i++)
+                    Marshal.WriteByte(ptr + i + 1, utf8Ptr[i]);


Yury-Fridlyand · 2025-03-11T22:13:29Z

Hi @X39, thank you so much for your contribution.
I agree with mentioned above about the need in splitting the PR. I also found few other things I'd like to highlight before I'm going into the depths of code.

.Net targeting and supported C# versions. The client should support .net8.0 (C# 12) 100% and probably .net6.0 (C# 10). There are ongoing discussions in that regard. .net9.0 won't have LTS, so we cannot rely on it.
3rd party dependencies. They are allowed, but they should comply with the whitelist - all they way down through the dependency tree. We have a tool that verifies that on every PR (ORT tool), but it is not activated for C#, because it wasn't planned to have any dependency.
Client API. It is required to have them as much similar as possible. A user should be able to switch from one language to another with minimal code changes. For sure, we have come language specific restrictions, but other than that they are identical. Docs are just copy-pasted and adopted.
It is especially relevant for commands with optional parameters (options). It is not scalable to do 2/4/8/16 signatures for a command with 1..4 optional arguments. Have a look at geosearch - it should be reflected by 2 client APIs, just because it has different return types, but not 22.

I'll do my best to help you with your code. I'll start with requirements and project standards - unfortunately, not all of them are easily accessible for the public.

You also need to downmerge in your branch some other fixes I'm working on once we complete them - routing (#3353) and handling binary strings (#3347). You can see the project plan for C# client in #216.

X39 · 2025-03-11T22:23:24Z

Feel free to close this PR or keep reviewing it as-is so I can integrate the changes once a final decision is made.

Over the weekend, I'll be splitting up the branch and creating sequential PRs that build on each other. I’ll aim to cover everything by then! :)

asafpamzn · 2025-03-12T06:51:52Z

Thanks a lot @X39 for this huge contribution we are very happy about it.
Lets see what is the best way to merge it in.
@Yury-Fridlyand and the other maintainers will help you to merge it in.

avifenesh · 2025-03-20T12:23:10Z

Well, I'm impressed!

X39 added 22 commits March 5, 2025 21:41

remove csharp folder

fb1afe5

BitCount implementation

6fbb87e

Update ReadMe

8cc9f70

Update .gitignore

9659b73

refactor and start set, yet to add tests

eacb456

Add blocking command functionality and improve exception handling

680bf5d

Introduces a new blocking `SendCommand` method in both the C# and Rust layers, with associated tests and FFI wiring.

Merge remote-tracking branch 'upstream/main' into feature/csharp

ad37a06

# Conflicts: # csharp/lib/BaseClient.cs # csharp/lib/Internals/Message.cs # csharp/lib/glide.csproj # csharp/rust/src/lib.rs # csharp/tests/Integration/TestConfiguration.cs # csharp/tests/tests.csproj

Adding a disappointing benchmark of 789.5 us

94be491

Fix callback in native client

42e98f6

reenable task bench

53b05b3

cleanup

ac4d37a

Update workflow

fd866e2

Loads of documentation and some minor API refactoring

5280f40

Added <GenerateDocumentationFile> and <IsPackable> properties to …

7af894d

…project files

Add ConnectionConfigBuilder for fluent connection configuration

7e8c8f4

Introduced `ConnectionConfigBuilder` and its associated extensions to simplify and enhance the process of configuring `ConnectionRequest` instances.

Add support for GET and started support for SET commands

f472cf0

Implemented functionality for executing GET and SET commands with extension methods in the Glide client. Added unit tests to ensure correct behavior for both existing and non-existent keys, as well as simple set operations.

X39 requested a review from a team as a code owner March 10, 2025 23:02

X39 changed the title ~~[WIP] CSharp Client~~ C#: [WIP] CSharp Client Mar 10, 2025

Yury-Fridlyand requested a review from Sa1Gur March 11, 2025 00:45

Yury-Fridlyand added the C# C# wrapper label Mar 11, 2025

Sa1Gur reviewed Mar 11, 2025

View reviewed changes

jbrinkman reviewed Mar 11, 2025

View reviewed changes

github-advanced-security bot found potential problems Mar 11, 2025

View reviewed changes

Yury-Fridlyand marked this pull request as draft March 11, 2025 22:31

tmp

cfd7ff2

X39 mentioned this pull request Mar 12, 2025

C#: Reorganize Structure #3362

Open

X39 added 6 commits March 15, 2025 11:43

tmp

9c8dbb5

update POC branch

bff6749

update POC branch

dbfaa33

fix benchmarkdotnet build

7613cc8

more benchmark and test stuff

9bc3b52

fix double free with bulkstring and refactor

ab766fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: [WIP] CSharp Client #3354

C#: [WIP] CSharp Client #3354

X39 commented Mar 10, 2025

nihohit commented Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

jbrinkman Mar 11, 2025

X39 Mar 11, 2025 •

edited

Loading

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

jbrinkman Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

Sa1Gur Mar 11, 2025

X39 Mar 11, 2025

Sa1Gur commented Mar 11, 2025

X39 commented Mar 11, 2025 •

edited

Loading

nihohit commented Mar 11, 2025

jbrinkman Mar 11, 2025

jbrinkman Mar 11, 2025

X39 Mar 11, 2025 •

edited

Loading

Yury-Fridlyand commented Mar 11, 2025

X39 commented Mar 11, 2025

asafpamzn commented Mar 12, 2025

avifenesh commented Mar 20, 2025


		namespace Valkey.Glide.InterOp.Native;

		[SuppressMessage("ReSharper", "InconsistentNaming")]

C#: [WIP] CSharp Client #3354

Are you sure you want to change the base?

C#: [WIP] CSharp Client #3354

Conversation

X39 commented Mar 10, 2025

About

Context

Remaining Work

Key Considerations for Merge

nihohit commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X39 Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sa1Gur commented Mar 11, 2025

X39 commented Mar 11, 2025 • edited Loading

nihohit commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X39 Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand commented Mar 11, 2025

X39 commented Mar 11, 2025

asafpamzn commented Mar 12, 2025

avifenesh commented Mar 20, 2025

X39 Mar 11, 2025 •

edited

Loading

X39 commented Mar 11, 2025 •

edited

Loading

X39 Mar 11, 2025 •

edited

Loading