feat(client): add callback/event mechanism between TypeScript and Go #2330

jyyi1 · 2025-01-15T21:09:09Z

This PR introduces a callback mechanism that allows TypeScript code to subscribe to events triggered in the Go code.

To demonstrate the functionality, this PR implements a VPN connection state change event. The TypeScript code can now subscribe to this event and receive updates on the VPN connection state.

This mechanism can be leveraged for both electron and the Cordova code.

Tested Outline-Client.deb on Ubuntu 22

client/electron/go_plugin.ts

fortuna

It seems you still need to update the PR to decouple the callback from the event handling. Let me know when I should take a look

client/electron/go_plugin.ts

sbruens

Nice. Just one small nit.

client/electron/go_plugin.ts

client/go/Taskfile.yml

fortuna · 2025-01-24T21:15:50Z

client/electron/vpn_service.ts

+ * Subscribes to all VPN connection related events.
+ * This function ensures that the subscription only happens once.
+ */
+async function subscribeVPNConnEvents(): Promise<void> {


Pass the VpnStatusCallback here and delete onVpnStatusChanged.

This eliminates the global state and makes the lifecycle/object relationship clearer. Easier to reason about.

Let the app worry about calling it once.

Added InitVpnService function to index.ts, so it have a chance to subscribe to the event only once when the electron app is ready.

fortuna · 2025-01-24T21:16:44Z

client/electron/vpn_service.ts

+const VPNConnDisconnecting: VPNConnStatus = 'Disconnecting';
+const VPNConnDisconnected: VPNConnStatus = 'Disconnected';
+
+interface VPNConn {


This is not a VPN connection: you can't read/write to it.

Perhaps call it VPNStatus?

VPNConnectionStatus or VPNConnectionState sounds good to me.

Renamed to VPNConnectionState to avoid confusion with VPNConnStatus.

fortuna · 2025-01-24T21:25:12Z

client/electron/go_plugin.ts

+ * Koffi requires us to register all persistent callbacks; we track the registrations here.
+ * @see https://koffi.dev/callbacks#registered-callbacks
+ */
+const koffiCallbacks = new Map<string, koffi.IKoffiRegisteredCallback>();


No need to change for this PR since it's already implemented, but there's an alternative design where the callback mapping you do in the Go code is done in the Typescript code instead. That's what Cordova does. This would circumvent the restrictions from koffi, since all you need in that case is a single callback.

Actually Koffi's limit is pretty big. We might not event need that much of callbacks in the entire system.

A maximum of 8192 callbacks can exist at the same time.

client/electron/vpn_service.ts

client/go/outline/callback/callback.go

client/electron/go_plugin.ts

fortuna · 2025-01-24T21:38:36Z

client/go/outline/event/event.go

+// RemoveListener removes a [callback.Callback] from the specified [EventName].
+//
+// Calling this function is safe even if the event has not been registered.
+func RemoveListener(evt EventName, cb callback.Token) {


I worry about global events. It hurts composability and reusability, as it's impossible to track who is firing what events. Different components may conflict with the same events (e.g. "OnError").

We need a design that can compartmentalize events.

For example, you could have functions specific for the VPN events (AddVPNAPIListener). Or have a way to create independent "channels" (AddListener(channel, event, callback)). The channel would have a unique id.

We also talked about having a SetVPNListener, which is easier to implement and reason about.

I would not worry too much about "tracking who is firing the event" because the trigger object can (and must) include itself in the event data. (Also OnError is a bad event, events should be concrete, like OnVPNConnectionConnectError, OnVPNConnectionDisconnectError, etc., the convention is On<ObjectType><EventName>, so actually no global events here).

The firing object should call the event handler with at least the following fields: { "this": "<object-id>", "data": "..." }.

The event handler can then distinguish the trigger object by looking at the event details and then distribute it accordingly in TypeScript. That's also why I included the VPNConn.ID in the event, it's not just an ID from the app code, it's also the ID which can uniquely identify the VPN connection object in Go.

I think it's ok, but the code safety and correctness relies on adhering to a strict structure that is not enforced by the compiler. A better approach would be to ensure we can create independent channels.

At a minimal create the object that represents the channel to stop using globals. This way we can have other methods that use their own channels.

Same question as the one in callback.go, do you mean a EventManager and a event.DefaultManager() method? But then will an EventManager also accepts a CallbackManager? This would add more unnecessary complexity to the code.

fortuna · 2025-01-28T00:13:43Z

client/electron/go_plugin.ts

+}
+
+/** Interface containing the exported native CGo functions. */
+interface CgoFunctions {


The encapsulation here is a bit off. It can be more object-oriented and remove globals.

Perhaps call this CallbackMap with public New and Delete methods.
Make the fields private.
Move koffiCallbacks into here.

The logic in ensureCgo can be in the constructor or factory function. Then there's no need to call ensureCgo in the methods.

Perhaps rename ensureCgo to getCallbackMap()

This makes the code more encapsulated, safer and with less globals.

It involves multiple components, I would prefer getBackendLib(), and returns an object with a methodChannel and a callbackManager object.

fortuna · 2025-01-28T00:16:00Z

client/electron/index.ts

@@ -471,6 +477,8 @@ function main() {
    // TODO(fortuna): Start the app with the window hidden on auto-start?
    setupWindow();

+    await initVpnService();


You can be more explicit. Like monitorVpnEvents. Or you could just inline the function here.

"init" doesn't say much, and it feels like you are doing much more than you actually are. It's also misleading, since some VPN initialization happen somewhere else

fortuna · 2025-01-28T00:19:20Z

client/go/outline/callback/callback.go

+
+var (
+	mu        sync.RWMutex
+	callbacks       = make(map[Token]Callback)


Same comment here. Create a callback map object, rather than using globals.

Do you mean introduce a CallbackManager and then have a default singleton method such as callback.DefaultManager() ?

fortuna · 2025-01-28T00:23:51Z

client/go/outline/event/event.go

+
+var (
+	mu        sync.RWMutex
+	listeners = make(map[EventName][]callback.Token)


Again, please avoid globals. Create an object to hold the fields and methods to manage the listeners.

fortuna · 2025-01-28T00:26:25Z

client/go/outline/event/event.go

+// RemoveListener removes a [callback.Callback] from the specified [EventName].
+//
+// Calling this function is safe even if the event has not been registered.
+func RemoveListener(evt EventName, cb callback.Token) {


I think it's ok, but the code safety and correctness relies on adhering to a strict structure that is not enforced by the compiler. A better approach would be to ensure we can create independent channels.

At a minimal create the object that represents the channel to stop using globals. This way we can have other methods that use their own channels.

jyyi1 · 2025-01-31T05:54:03Z

Hi @fortuna @sbruens , I simplified the code by removing the event package, also exposing VPN state change event directly via methodChannel, and encapsulating callback managers in Go & TS and backend channel in TS. Please take another look. Thanks!

feat(client): add callback/event mechanism between TypeScript and Go

873296c

github-actions bot added the size/L label Jan 15, 2025

jyyi1 and others added 4 commits January 15, 2025 16:30

Add corresponding cgo code to TS

12cb5f5

Merge branch 'master' into junyi/vpn-callback-linux

5031d15

Expose C functions to subscribe/unsubcribe events

af81acc

raise and subscribe to VPN connection status change event

15e1735

jyyi1 marked this pull request as ready for review January 16, 2025 03:17

jyyi1 requested review from a team as code owners January 16, 2025 03:17

github-actions bot added size/XL and removed size/L labels Jan 16, 2025

jyyi1 requested review from fortuna and sbruens January 16, 2025 03:18

Simplify parseVPNConnJSON

4a2c58d

sbruens reviewed Jan 16, 2025

View reviewed changes

client/electron/go_plugin.ts Outdated Show resolved Hide resolved

client/electron/go_plugin.ts Outdated Show resolved Hide resolved

fortuna reviewed Jan 17, 2025

View reviewed changes

client/electron/go_plugin.ts Outdated Show resolved Hide resolved

client/electron/go_plugin.ts Outdated Show resolved Hide resolved

decouple callback from event

3ced93e

jyyi1 force-pushed the junyi/vpn-callback-linux branch from f617f0b to 3ced93e Compare January 18, 2025 01:58

jyyi1 added 3 commits January 21, 2025 18:36

expose new callback function in electron Go plugin

cd3129d

Merge branch 'master' into junyi/vpn-callback-linux

d653dce

Use the new callback in TypeScript

5a24569

jyyi1 requested review from fortuna and sbruens January 22, 2025 01:26

sbruens approved these changes Jan 22, 2025

View reviewed changes

client/electron/go_plugin.ts Outdated Show resolved Hide resolved

test latest go for CI

055c1e3

jyyi1 commented Jan 22, 2025

View reviewed changes

client/go/Taskfile.yml Outdated Show resolved Hide resolved

jyyi1 added 4 commits January 22, 2025 18:58

try gnu clib

22ebf3c

more debug for zig

8c4ffac

Try newer action runner

d72486b

try mingw

4429a10

jyyi1 and others added 4 commits January 23, 2025 15:00

revert the CI job changes and clear cache

93496cb

add callback Token type to TypeScript

f33903e

fix linting errors

2e126df

Merge branch 'master' into junyi/vpn-callback-linux

dab470e

fortuna requested changes Jan 24, 2025

View reviewed changes

jyyi1 added 3 commits January 27, 2025 15:32

Use callback Token directly

5080879

refactor VPN status changed event

4e3e120

Fix linting

b8e738f

jyyi1 requested review from fortuna and sbruens January 27, 2025 21:51

fortuna reviewed Jan 28, 2025

View reviewed changes

jyyi1 added 4 commits January 30, 2025 23:38

remove event package, replace it directly with callback

a3de633

fix koffi async call

535d0c4

Merge branch 'master' into junyi/vpn-callback-linux

64bd661

resolve conflicts

75d5a45

jyyi1 requested a review from fortuna January 31, 2025 05:54

update comment

ca2b263

jyyi1 mentioned this pull request Jan 31, 2025

Rewrite Outline Client's Routing for Linux #2312

Open

14 tasks

fortuna approved these changes Jan 31, 2025

View reviewed changes

sbruens approved these changes Jan 31, 2025

View reviewed changes

Merge branch 'master' into junyi/vpn-callback-linux

c46457e

jyyi1 merged commit de32ebc into master Feb 1, 2025
25 checks passed

jyyi1 deleted the junyi/vpn-callback-linux branch February 1, 2025 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): add callback/event mechanism between TypeScript and Go #2330

feat(client): add callback/event mechanism between TypeScript and Go #2330

jyyi1 commented Jan 15, 2025 •

edited

Loading

fortuna left a comment

sbruens left a comment

fortuna Jan 24, 2025

jyyi1 Jan 27, 2025

fortuna Jan 24, 2025

jyyi1 Jan 24, 2025

jyyi1 Jan 27, 2025

fortuna Jan 24, 2025

jyyi1 Jan 27, 2025

fortuna Jan 24, 2025

fortuna Jan 24, 2025

jyyi1 Jan 24, 2025 •

edited

Loading

fortuna Jan 28, 2025

jyyi1 Jan 28, 2025 •

edited

Loading

fortuna Jan 28, 2025

jyyi1 Jan 28, 2025 •

edited

Loading

fortuna Jan 28, 2025

fortuna Jan 28, 2025

jyyi1 Jan 28, 2025

fortuna Jan 28, 2025

fortuna Jan 28, 2025

jyyi1 commented Jan 31, 2025

feat(client): add callback/event mechanism between TypeScript and Go #2330

feat(client): add callback/event mechanism between TypeScript and Go #2330

Conversation

jyyi1 commented Jan 15, 2025 • edited Loading

fortuna left a comment

Choose a reason for hiding this comment

sbruens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 commented Jan 31, 2025

jyyi1 commented Jan 15, 2025 •

edited

Loading

jyyi1 Jan 24, 2025 •

edited

Loading

jyyi1 Jan 28, 2025 •

edited

Loading

jyyi1 Jan 28, 2025 •

edited

Loading