Skip to content

Conversation

@Dm17r1y
Copy link

@Dm17r1y Dm17r1y commented Oct 6, 2025

Added C#/dotnet bindings for turso

@Dm17r1y Dm17r1y requested a review from penberg as a code owner October 6, 2025 11:10
@penberg penberg changed the title Dotnet bindings for turso Add dotnet bindings to Turso Oct 6, 2025
@jibsen-vh jibsen-vh mentioned this pull request Oct 7, 2025
Copy link

@alsi-lawr alsi-lawr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 2 separate libraries to have proper separation of concerns: 1. A raw bindings library (think SQLiteRawPCL, like Turso.Raw) that has the P/Invoke calls internal to the library and a publicly shipped API that maps the unmanaged types to managed C# types (where appropriate).
2. The actual ADO.NET provider for turso

I'd suggest something like Turso.Raw and Turso.Data for the two libraries. There's a lot of misunderstanding of how to safely do interop between unmanaged and managed langs, but that should be relatively straight forward to iron out.

using var command = connection.CreateCommand();
command.CommandText = "SELECT * FROM t;";
using var reader = command.ExecuteReader();
while (reader.Read());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JIT is very likely to completely optimize out most of this loop since the reader.Read values are just being discarded. You have to do something with the value to force the JIT to evaluate this.

Suggest doing something like:

int sum = 0;
while(reader.Read())
    sum += reader.GetInt32(0);

GC.KeepAlive(sum);

Also, I'd suggest forcing this inline and making it static so you can minimise the overhead of the function call itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alsi-lawr, thank you for the review

Let's prevent JIT optimization here. I wrote code with a sum variable, added MethodImplOptions.AggressiveInlining and made it static


namespace Turso.Native;

public class TursoNativeDatabase : IDisposable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these mid-level "native" abstractions are redundant. These do not add actual type safety, that should be done in the core bindings that are being exposed. Any handles you've done in here should just be moved to the actual database abstraction that should be what users consume. No one should be using this, and including it in both the publicly shipped API and the library more generally is confusing (how does a user decide whether to use a TursoValue or a TursoNativeValue?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, these abstractions shouldn't be accessible to the public

So I added a Turso.Raw project, moved all P/Invoke code to that project and made some abstractions like TursoNativeValue internal

Now the Turso project remains only ADO.NET classes

Comment on lines 13 to 14
fixed(TursoErrorHandle* errorHandlePtr = &this)
return (IntPtr)errorHandlePtr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only fixes the memory address of the error handle in the scope of the fixed block. Once you've left this block, there is no guarantee that the stack address for this value remains fixed. Passing this then to unmanaged code is completely undefined behaviour, and can cause rewrites of arbitrary data on the stack.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this: stack memory is never relocated, that's my bad. There is no guarantee that this will be stackallocated, however, and is a useless operation. It's still very bad practice to pass the pointer from managed to unmanaged like this when it's unnecessary. Why instantiate in managed c# when you can just as easily just let the argument be an out? Why have 2 steps of instantiating the pointer and passing it to rust just to immediately overwrite the data at that address, when you can just let the unmanaged rust code do it all in one place?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that redunant

Error handling with out Intptr error parameter just simplier, so I rewrote that to recieving errors via out parameter

private const string DllName = "turso_dotnet.dll";

[DllImport(DllName, EntryPoint = "db_open", CallingConvention = CallingConvention.Cdecl)]
public static extern IntPtr OpenDatabase(string path, IntPtr errorHandle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very dangerous way of setting data from unmanaged code. .NET already deals with this for you. You can change this signature to

    [DllImport(DllName, EntryPoint = "db_open", CallingConvention = CallingConvention.Cdecl)]
    public static extern IntPtr db_open(string path, out IntPtr errorHandlePtr); // move this to a static class "TursoInterop" or something along those lines

A good structured approach would be to have the raw P/Invoke calls left bare and internal, and absolutely leave the pointer assignments to the unmanaged code. Then have a managed wrapper function for each call that converts it into the C# data types, like:

public static class TursoBindings
{
    public static IntPtr OpenDatabase(string path); // this will throw on failed connection
}

This can just be used within the actual ADO.NET DbConnection wrapper without a TursoNativeDatabase abstraction, since you're never actually representing the Database struct in the managed C# at all. You just need to pass the handle around (as a SafeHandle -- look into why this is used pls).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, using SafeHandle is much better

Here I wrote a TursoDatabaseHandle and TursoStatementHandle which hold databasePtr and statementPtr

Now TursoBindings wraps raw IntPtr and returns an actual SafeHandle

public static class TursoBindings
{
    public static TursoDatabaseHandle OpenDatabase(string path);
}

Comment on lines 5 to 13
[StructLayout(LayoutKind.Explicit)]
public struct TursoNativeArray
{
[FieldOffset(0)]
public IntPtr Data;

[FieldOffset(8)]
public UInt64 Length;
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not safe. IntPtr is not guaranteed to be 8 bytes, neither on the rust side *const u8 or on the C# side. It's platform dependent, and will have a different offset on 32 bit and 64 bit platforms. You've hard-coded the offsets for 64 bit only. Just make it sequential, C# will very happily map the byte layout of the rust struct to this struct in a platform-dependent way for you.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's made it StructLayout(LayoutKind.Sequential)

@penberg penberg mentioned this pull request Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants