refactor(connlib): remove Result return values from callbacks

[thomaseizinger]

Currently, an error returned by Tunnel::poll_next_event is only logged. In other words, they are never fatal. This creates a tricky to understand relationship on what kind of errors should be returned from callbacks. Because connlib is used on multiple operating systems, it has no idea how fatal a particular error is.

This PR removes all of these Result return values with the following consequences:

• For Android, we now panic when a callback fails. This is a slight change in behaviour. I believe that previously, any exception thrown by a callback into Android was caught and returned as an error. Now, we panic because in the FFI layer, we don't have any information on how fatal the error is. For non-fatal errors, the Android app should simply not throw an exception. The panics will cause the connlib task to be shut down which triggers an on_disconnect.
• For Swift, there is no behaviour change. The FFI layer already did not support Results for those callbacks. I don't know how exceptions from Swift are translated across the FFI layer but there is no change to what we had before.
• For the Tauri client:
  • I chose to log errors on ERROR level and continue gracefully for the DNS resolvers.
  • We panic in case the controller channel is full / closed. That should really never happen in practice though unless we are currently shutting down the app.

Resolves: #4064.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
firezone	⬜️ Ignored (Inspect)	Visit Preview		Mar 20, 2024 1:57am

[github-actions]

Terraform Cloud Plan Output

Plan: 9 to add, 8 to change, 9 to destroy.

Terraform Cloud Plan

[thomaseizinger]

I think conceptually, we should adopt the following model:

1. Any exception / panic in a callback will shut down connlib. I'd consider that to be reasonable and not surprising.
2. If a client encounters a fatal error that needs to stop connlib, it should call disconnect.

(1) should only be considered a last-resort way of dealing with things, i.e. exceptions / panics shouldn't be the standard way of error handling.

[github-actions]

Performance Test Results

TCP

Test Name	Received/s	Sent/s	Retransmits
direct-tcp-client2server	224.4 MiB (+3%)	226.2 MiB (+3%)	382 (+75%)
direct-tcp-server2client	227.2 MiB (+0%)	228.9 MiB (+0%)	559 (+69%)
relayed-tcp-client2server	151.1 MiB (+2%)	152.0 MiB (+2%)	148 (-10%)
relayed-tcp-server2client	154.8 MiB (-2%)	155.2 MiB (-2%)	209 (+19%)

UDP

Test Name	Total/s	Jitter	Lost
direct-udp-client2server	50.0 MiB (+0%)	0.20ms (+412%)	0.00% (NaN%)
direct-udp-server2client	50.0 MiB (+0%)	0.01ms (+70%)	0.00% (NaN%)
relayed-udp-client2server	50.0 MiB (+0%)	0.11ms (-20%)	0.00% (NaN%)
relayed-udp-server2client	50.0 MiB (-0%)	0.04ms (-42%)	0.00% (NaN%)

[ReactorScram]

With both this and #4159, I'm more confused about how the callbacks are intended to work.

I'll add to my comment in the Slack thread about 4159

[thomaseizinger]

With both this and #4159, I'm more confused about how the callbacks are intended to work.

I'll add to my comment in the Slack thread about 4159

Mind re-reviewing this after the conversation we had on Slack? :)

[ReactorScram]

LGTM. Hopefully the rest of the issues with callbacks get fixed some day

[thomaseizinger]

@conectado Do you have any opinions on this or can I merge?