Skip to content

Fix connecting via SenderOptions.addr #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rob-Hague wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix connecting via SenderOptions.addr #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/net-questdb-client-tests/MultiUrlHttpTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,23 @@ public void ParseMultipleAddresses_FromConfigString()
{
// Test parsing multiple addresses from config string
var options =
new SenderOptions("http::addr=localhost:9000;addr=localhost:9001;addr=localhost:9002;auto_flush=off;");
new SenderOptions("http::addr=localhost:9001;aDdR=locALhOSt:9002;addr=localhost:9003;auto_flush=off;");

Assert.That(options.addr, Is.EqualTo("localhost:9001"));
Assert.That(options.AddressCount, Is.EqualTo(3));
Assert.That(options.addresses[0], Is.EqualTo("localhost:9000"));
Assert.That(options.addresses[1], Is.EqualTo("localhost:9001"));
Assert.That(options.addresses[2], Is.EqualTo("localhost:9002"));
Assert.That(options.addresses[0], Is.EqualTo("localhost:9001"));
Assert.That(options.addresses[1], Is.EqualTo("locALhOSt:9002"));
Assert.That(options.addresses[2], Is.EqualTo("localhost:9003"));
}

[Test]
public void ParseMultipleAddresses_DefaultsToSingleAddress()
{
// Test that single address is handled correctly
var options = new SenderOptions("http::addr=localhost:9000;auto_flush=off;");
var options = new SenderOptions("http::addr=localhost:9999;auto_flush=off;");

Assert.That(options.AddressCount, Is.EqualTo(1));
Assert.That(options.addresses[0], Is.EqualTo("localhost:9000"));
Assert.That(options.addresses[0], Is.EqualTo("localhost:9999"));
}

[Test]
Expand All @@ -69,7 +70,8 @@ public void ParseMultipleAddresses_NoAddrSpecified()
// Test that default address is used when none specified
var options = new SenderOptions("http::auto_flush=off;");

Assert.That(options.AddressCount, Is.GreaterThan(0));
Assert.That(options.addr, Is.EqualTo("localhost:9000"));
Assert.That(options.AddressCount, Is.EqualTo(1));
Assert.That(options.addresses[0], Is.EqualTo("localhost:9000"));
}

Expand Down
37 changes: 32 additions & 5 deletions src/net-questdb-client-tests/SenderOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ public void NoPortProvided()
public void BasicParse()
{
Assert.That(
new SenderOptions("http::addr=localhost:9000;").addr,
Is.EqualTo("localhost:9000"));
new SenderOptions("http::addr=localhost:9999;").addr,
Is.EqualTo("localhost:9999"));
}

[Test]
Expand All @@ -59,10 +59,37 @@ public void CapitalCaseInValues()
[Test]
public void DuplicateKey()
{
// duplicate keys are 'last writer wins'
// duplicate (non-addr) keys are 'last writer wins'
Assert.That(
new SenderOptions("http::addr=localhost:9000;addr=localhost:9009;").addr,
Is.EqualTo("localhost:9009"));
new SenderOptions("http::auth_timeout=15000;auth_timeout=8000;").auth_timeout,
Comment on lines +62 to +64
Copy link
Author

@Rob-Hague Rob-Hague Feb 7, 2026

Choose a reason for hiding this comment

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

This test is at odds with how addr is documented ("When multiple addresses are configured, this returns the first one."). So either the test needs to change or addr does. Went with the former

Is.EqualTo(TimeSpan.FromMilliseconds(8000)));
}

[Test]
public void DefaultConstructorInit()
{
var options = new SenderOptions
{
addr = "192.168.0.218:9000",
};

Assert.That(options.addr, Is.EqualTo("192.168.0.218:9000"));
Assert.That(options.AddressCount, Is.EqualTo(1));
Assert.That(options.addresses[0], Is.EqualTo("192.168.0.218:9000"));
}

[Test]
public void SetGetAddr()
{
var options = new SenderOptions();

Assert.That(options.addr, Is.EqualTo("localhost:9000"));

options.addr = "192.168.0.218:9000";

Assert.That(options.addr, Is.EqualTo("192.168.0.218:9000"));
Assert.That(options.AddressCount, Is.EqualTo(1));
Assert.That(options.addresses[0], Is.EqualTo("192.168.0.218:9000"));
}

[Test]
Expand Down
47 changes: 22 additions & 25 deletions src/net-questdb-client/Utils/SenderOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ public record SenderOptions
"pool_timeout", "tls_verify", "tls_roots", "tls_roots_password", "own_socket", "gzip",
};

private string _addr = "localhost:9000";
private List<string> _addresses = new();
private const string DefaultAddress = "localhost:9000";
private readonly List<string> _addresses = new();
Comment on lines +59 to +60
Copy link

@coderabbitai coderabbitai bot Feb 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find and examine the SenderOptions.cs file
fd -t f "SenderOptions.cs" --type f

Repository: questdb/net-questdb-client

Length of output: 115

🏁 Script executed:

# Get the file size and structure
wc -l src/net-questdb-client/Utils/SenderOptions.cs

Repository: questdb/net-questdb-client

Length of output: 119

🏁 Script executed:

# Check the record declaration and key lines
sed -n '1,70p' src/net-questdb-client/Utils/SenderOptions.cs

Repository: questdb/net-questdb-client

Length of output: 2817

🏁 Script executed:

# Check the addr property (around lines 162-167)
sed -n '160,175p' src/net-questdb-client/Utils/SenderOptions.cs

Repository: questdb/net-questdb-client

Length of output: 529

🏁 Script executed:

# Check the WithClientCert method (around line 731)
sed -n '725,740p' src/net-questdb-client/Utils/SenderOptions.cs

Repository: questdb/net-questdb-client

Length of output: 380

🏁 Script executed:

# Search for all 'with' expressions in C# files
rg -nP --type=cs '\bwith\s*\{' --glob '!**/obj/**' --glob '!**/bin/**'

Repository: questdb/net-questdb-client

Length of output: 214

🏁 Script executed:

# Check how many lines the file has and look at the end for any relevant code
tail -20 src/net-questdb-client/Utils/SenderOptions.cs

Repository: questdb/net-questdb-client

Length of output: 525

🏁 Script executed:

# Verify field initializer behavior in records and check if there's any evidence of actual mutations
rg -nP --type=cs '_addresses' src/net-questdb-client/Utils/SenderOptions.cs -B2 -A2

Repository: questdb/net-questdb-client

Length of output: 1477

🏁 Script executed:

# Check if there are usages of WithClientCert in tests or elsewhere that might expose the bug
rg -nP --type=cs 'WithClientCert' --glob '!**/obj/**' --glob '!**/bin/**' -B3 -A3

Repository: questdb/net-questdb-client

Length of output: 1297

🏁 Script executed:

# Check what tests exist and if they might be hitting this aliasing issue
rg -nP --type=cs '\.addr\s*=' --glob '!**/obj/**' --glob '!**/bin/**' -B2 -A2

Repository: questdb/net-questdb-client

Length of output: 520

Mutable List<string> field in record creates aliasing hazard with with expressions.

SenderOptions is a record type (line 43), so the compiler-synthesized copy constructor performs shallow copy of all fields. This means any instance created via with expression (e.g., WithClientCert at line 731) shares the same _addresses list reference. If the original or copy subsequently sets addr property, the setter's _addresses.Clear() call (line 165) mutates the shared list, corrupting the other instance's state.

Applies to lines 60 (field declaration), 162-167 (addr property), and 729-733 (WithClientCert).

Recommended fixes:

  1. Clone _addresses in a custom copy constructor.
  2. Replace List<string> with ImmutableList<string> and update the setter to reassign instead of mutate.
  3. Change SenderOptions from record to class if value semantics are not required.
🤖 Prompt for AI Agents 
In `@src/net-questdb-client/Utils/SenderOptions.cs` around lines 59 - 60,
SenderOptions (record) has a mutable List<string> _addresses which is
shallow-copied by the compiler-generated copy constructor and causes aliasing
when using with-expressions (affecting addr setter and WithClientCert); fix by
adding a custom copy constructor for SenderOptions that deep-clones _addresses
(e.g., create a new List<string> from other._addresses) so copies get
independent lists, and ensure any methods/properties that mutate _addresses
(addr property setter) operate on the instance's private list; alternatively,
replace the field with an immutable collection type (e.g.,
ImmutableList<string>) and change addr setter to reassign a new immutable list
instead of calling _addresses.Clear(), then adjust WithClientCert to rely on the
safe copy semantics.

private TimeSpan _authTimeout = TimeSpan.FromMilliseconds(15000);
private AutoFlushType _autoFlush = AutoFlushType.on;
private int _autoFlushBytes = int.MaxValue;
Expand Down Expand Up @@ -91,7 +91,7 @@ public record SenderOptions
/// </summary>
public SenderOptions()
{
ParseAddresses();
addr = DefaultAddress;
}

/// <summary>
Expand All @@ -103,8 +103,6 @@ public SenderOptions(string confStr)
ReadConfigStringIntoBuilder(confStr);
ParseEnumWithDefault(nameof(protocol), "http", out _protocol);
ParseEnumWithDefault(nameof(protocol_version), "auto", out _protocol_version);
ParseStringWithDefault(nameof(addr), "localhost:9000", out _addr!);
ParseAddresses();
ParseEnumWithDefault(nameof(auto_flush), "on", out _autoFlush);
ParseIntThatMayBeOff(nameof(auto_flush_rows), IsHttp() ? "75000" : "600", out _autoFlushRows);
ParseIntThatMayBeOff(nameof(auto_flush_bytes), int.MaxValue.ToString(), out _autoFlushBytes);
Expand Down Expand Up @@ -161,8 +159,12 @@ public ProtocolVersion protocol_version
/// </remarks>
public string addr
{
get => _addr;
set => _addr = value;
get => _addresses[0];
set
{
_addresses.Clear();
_addresses.Add(value);
}
}

/// <summary>
Expand Down Expand Up @@ -595,21 +597,25 @@ private void ReadConfigStringIntoBuilder(string confStr)
// Parse addresses manually before using DbConnectionStringBuilder
// because DbConnectionStringBuilder only keeps the last value for duplicate keys
_addresses.Clear();
foreach (var param in paramString.Split(';'))
foreach (var param in paramString.Split(';', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries))
{
if (string.IsNullOrWhiteSpace(param)) continue;
if (!param.StartsWith("addr", StringComparison.OrdinalIgnoreCase))
{
continue;
}

var kvp = param.Split('=');
if (kvp.Length == 2 && kvp[0].Trim() == "addr")
var kvp = param.Split('=', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries);
if (kvp.Length == 2 && kvp[0].Equals("addr", StringComparison.OrdinalIgnoreCase))
{
var addrValue = kvp[1].Trim();
if (!string.IsNullOrEmpty(addrValue))
{
_addresses.Add(addrValue);
}
_addresses.Add(kvp[1]);
}
}

if (_addresses.Count == 0)
{
_addresses.Add(DefaultAddress);
}

_connectionStringBuilder = new DbConnectionStringBuilder
{
ConnectionString = paramString,
Expand Down Expand Up @@ -704,15 +710,6 @@ private void VerifyCorrectKeysInConfigString()
}
}

private void ParseAddresses()
{
// If no addresses were parsed from config string, use the primary addr
if (_addresses.Count == 0)
{
_addresses.Add(_addr);
}
}

/// <summary>
/// Construct a new <see cref="ISender" /> from the current options.
/// </summary>
Expand Down