|
| 1 | +# Future Improvements |
| 2 | + |
| 3 | +## Polish |
| 4 | + |
| 5 | +### Safe (non-breaking) |
| 6 | + |
| 7 | +**1. Add logging to `SubscribeWithSnapshotAsync` in `LiveClient.cs`** |
| 8 | +- File: `src/Databento.Client/Live/LiveClient.cs` (lines ~327-370) |
| 9 | +- `SubscribeAsync` logs on entry and success. `SubscribeWithSnapshotAsync` has zero logging. |
| 10 | +- Fix: Add `_logger.LogInformation(...)` for entry and success to match `SubscribeAsync` pattern. |
| 11 | + |
| 12 | +**2. Inline unnecessary local variable in `BlockUntilStoppedAsync` in `LiveClient.cs`** |
| 13 | +- File: `src/Databento.Client/Live/LiveClient.cs` (line ~880) |
| 14 | +- `var streamTask = Interlocked.CompareExchange(ref _streamTask, null, null);` assigns to a variable only used for a null check. |
| 15 | +- Fix: Change to `if (Interlocked.CompareExchange(ref _streamTask, null, null) == null)`. |
| 16 | +- Note: Same pattern exists in the `BlockUntilStoppedAsync(TimeSpan timeout, ...)` overload. |
| 17 | + |
| 18 | +### Potentially Breaking |
| 19 | + |
| 20 | +**3. Add `ConfigureAwait(false)` to `StreamAsync` in `LiveClient.cs`** |
| 21 | +- File: `src/Databento.Client/Live/LiveClient.cs` (line ~615) |
| 22 | +- `await foreach (var record in _recordChannel.Reader.ReadAllAsync(cancellationToken))` is missing `.ConfigureAwait(false)`. |
| 23 | +- Standard best practice for library code — not having it can cause deadlocks in environments with a `SynchronizationContext`. |
| 24 | +- **Why breaking:** Changes thread affinity for callers using the library in WPF, WinForms, or ASP.NET apps with a `SynchronizationContext`. Their `await foreach` would resume on a thread pool thread instead of their original context. We don't know all use cases of this library. |
| 25 | + |
| 26 | +**4. Remove `async`/`await Task.CompletedTask` from `ResubscribeAsync` in `LiveClient.cs`** |
| 27 | +- File: `src/Databento.Client/Live/LiveClient.cs` (lines ~568-586) |
| 28 | +- Method is marked `async` and ends with `await Task.CompletedTask`. The `async` keyword generates a state machine for no reason — the method is entirely synchronous (P/Invoke call). |
| 29 | +- Correct pattern: remove `async`, return `Task.CompletedTask` (same as `SubscribeAsync` does). |
| 30 | +- **Why breaking:** With `async`, exceptions from the P/Invoke are captured into the returned `Task` and thrown at the `await` site. Without `async`, exceptions throw synchronously at the call site. For callers who `await` immediately, no difference. For callers who capture the `Task` and `await` later, the exception timing changes. |
| 31 | + |
| 32 | +**5. Replace `ConcurrentBag` with `List` for `_subscriptions` in `LiveClient.cs`** |
| 33 | +- File: `src/Databento.Client/Live/LiveClient.cs` (line ~38) |
| 34 | +- `ConcurrentBag` was a defensive choice. All writes (`_subscriptions.Add`) happen during `SubscribeAsync` which is synchronous and completes before `StartAsync`. After `Start`, the collection is read-only. `List` is safe for concurrent reads with no concurrent writes. |
| 35 | +- databento-cpp uses a plain `std::vector` — `ConcurrentBag` is a fidelity divergence. |
| 36 | +- The comment "HIGH FIX: Use thread-safe collection for concurrent subscription operations" was added during a code quality pass but the concurrent write scenario doesn't materialize in the current code. `ResubscribeAsync` uses native resubscribe (`dbento_live_resubscribe`), not the managed collection. |
| 37 | +- `ConcurrentBag` is on the cold setup path (not the hot data path) and has zero overhead when uncontended, so it's harmless. It differs from `SemaphoreSlim` (which would serialize P/Invoke operations on the hot path). |
| 38 | +- **Why breaking:** If a code path exists that we haven't identified where a background thread accesses `_subscriptions`, replacing with `List` introduces a race condition. Requires thorough analysis and testing before changing. |
| 39 | + |
| 40 | +### Cosmetic (low priority) |
| 41 | + |
| 42 | +**6. Shorten `Models.Dbn.DbnMetadata` references in `LiveClient.cs`** |
| 43 | +- File: `src/Databento.Client/Live/LiveClient.cs` |
| 44 | +- Fully qualified `Models.Dbn.DbnMetadata` is used throughout. Could add a `using` directive and use `DbnMetadata`. |
| 45 | +- Purely cosmetic, zero runtime impact. Not worth the churn unless touching the file for other reasons. |
0 commit comments