Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: implement Speaker and Serdes #6

Merged
merged 8 commits into from
Nov 27, 2024
Merged

feat: implement Speaker and Serdes #6

merged 8 commits into from
Nov 27, 2024

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Nov 21, 2024

Closes #3

  • Adds protobuf code generation
  • Adds utilities and wrapping code around RPC message types
  • Adds RPC protocol code required for sending and receiving messages
  • Adds a end-to-end test with messages and replies between two Speakers

TODO:

  • Add the csharp_namespace option to the upstream vpn.proto
  • Decide between event delegates (callbacks) or channels for receiving messages
  • Plumb CancellationTokenSource into more operations (any async Speaker operation should use the supplied CancellationToken as well as the global CancellationTokenSource)
  • Make the startup process async?
  • Try to declutter the generic type specifications around the codebase
    • It may be possible to condense the two TS/TSi and TR/TRi into just TS and TR with some finangling and changes around the RpcMessage<T> type
      • Maybe switch (back) to an interface rather than an abstract class
  • Move the role to be attached to the type so it doesn't need to be passed in manually to the Speaker at all
  • Tests
    • ApiVersion
    • RpcRole
    • RpcHeader
    • RpcMessage (and RpcRoleAttribute)
    • Serdes
    • Speaker

- Adds protobuf code generation
- Adds utilities and wrapping code around RPC message types
- Adds RPC protocol code required for sending and receiving messages
- Adds a end-to-end test with messages and replies between two Speakers
@deansheather deansheather self-assigned this Nov 21, 2024
The RpcMessage<T> type is now implemented directly on the protobuf types using partial classes.
@deansheather deansheather marked this pull request as ready for review November 26, 2024 04:34
Copy link
Member

@ethanndickson ethanndickson left a comment

Choose a reason for hiding this comment

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

I'm happy with the repo structure, the namespaces, and I of course agree that the C# code should be idiomatic C# and not just a translation of the Go code. FWICT the C# API looks good.

LGTM, aside from the earlier RPC.MsgID thread - can probably just merge this for now?

/// A task that completes when all tasks are completed, with the cancellation or exception state of the first
/// non-successful task
/// </returns>
public static async Task CancellableWhenAll(CancellationTokenSource cts, params Task[] tasks)
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Copy link
Collaborator

@spikecurtis spikecurtis left a comment

Choose a reason for hiding this comment

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

Some inline comments, but overall, I like the shape of the APIs to the protocol.

@deansheather deansheather merged commit 08487ac into main Nov 27, 2024
@deansheather deansheather deleted the dean/speaker branch December 9, 2024 09:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the CoderVPN protocol
3 participants