C# implementation of Transactions for Procedures #3809
Conversation
…ession test server
gefjon
left a comment
gefjon
left a comment
There was a problem hiding this comment.
I'd like to see a more in-depth PR description which goes into detail on your design and implementation choices. Especially in big PRs like this, there's a burden on the author to both keep the changes as contained and laser-focused as possible, and also to proactively explain to reviewers what's going on in the diff. Anywhere I've left a comment asking for reasoning, that's the kind of thing that should be in your PR description proactively.
| /// <summary> | ||
| /// Indicates whether this type represents a void return. | ||
| /// </summary> | ||
| public virtual bool IsVoid => false; |
There was a problem hiding this comment.
Distinguishing between void and unit returns feels weird and wrong to me. Why is this necessary?
There was a problem hiding this comment.
C#'s type system treats void and unit (ValueTuple) as distinct types. As part of serialization, we need to know whether to expect a return value, and C# requires explicit handling of this.
| public partial struct AggregateElement | ||
| public partial struct AggregateElement(string name, AlgebraicType algebraicType) | ||
| { | ||
| public string? Name; | ||
| public string? Name = name; | ||
|
|
||
| public AlgebraicType AlgebraicType; | ||
|
|
||
| public AggregateElement(string name, AlgebraicType algebraicType) | ||
| { | ||
| Name = name; | ||
| AlgebraicType = algebraicType; | ||
| } |
There was a problem hiding this comment.
C# benefits from property initializers and explicit constructors for better tooling support. Basically this pattern matches .NET's design guidelines and enables better encapsulation.
| new Timestamp(checked(point.MicrosecondsSinceUnixEpoch - interval.Microseconds)); | ||
|
|
||
| public int CompareTo(Timestamp that) | ||
| public readonly int CompareTo(Timestamp that) |
There was a problem hiding this comment.
Why'd this change? Is it related to this PR?
There was a problem hiding this comment.
Na, this was just an IDE recommendation that made sense to me when I was making changes. It's not required. It just helps the compiler optimize the method.
| /// Read-only database access for view contexts. | ||
| /// The code generator will extend this partial class to add table accessors. | ||
| /// </summary> | ||
| public sealed partial class LocalReadOnly |
There was a problem hiding this comment.
Why did this change? LocalReadOnly is for views, which should be unrelated to this procedure PR, right?
| /// This class is responsible for maintaining the current procedure context and | ||
| /// managing transaction state. | ||
| /// </summary> | ||
| public class ProcedureContextManager : IProcedureContextManager |
There was a problem hiding this comment.
I don't understand why this context manager is necessary, or why we need IDisposables to manage a stack of scopes. The Rust version you linked to in #3638 doesn't appear to contain anything similar.
| bool CommitMutTxWithRetry(Func<bool> retryBody); | ||
|
|
||
| /// <summary>Asynchronously commits a transaction with a retry mechanism.</summary> | ||
| Task<bool> CommitMutTxWithRetryAsync(Func<Task<bool>> retryBody); |
There was a problem hiding this comment.
Why do we have async stuff happening inside of module code? No other module languages currently have async inside of them, as our syscalls are blocking.
| /// The procedure event that caused this callback to run. | ||
| /// </summary> | ||
| public readonly ProcedureEvent Event; | ||
| public ProcedureEvent Event { get; } |
There was a problem hiding this comment.
Why do we have changes in this file? I'd expect this PR not to affect client codegen, or anything about the client SDK, at all.
| var entity1_radius = MassToRadius(entity1.mass); | ||
| var entity2_radius = MassToRadius(entity2.mass); | ||
| var distance = (float) |
There was a problem hiding this comment.
These changes appear unrelated to this PR.
| using SpacetimeDB; | ||
|
|
||
| namespace Benchmarks; | ||
| using SpacetimeDB; |
There was a problem hiding this comment.
Changes in this file also appear unrelated to this PR.
| using SpacetimeDB; | ||
|
|
||
| namespace Benchmarks; | ||
| using SpacetimeDB; |
There was a problem hiding this comment.
More apparently unrelated changes.
Description of Changes
Implements the C# equivalent of #3638
API and ABI breaking changes
Should not be a breaking change
Expected complexity level and risk
2
Testing