Page MenuHomePhabricator

[Tunnelbroker] refactor initializing session to make it possible to send a response to the client on session creation failure
ClosedPublic

Authored by kamil on Oct 25 2023, 3:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 9:42 AM
Unknown Object (File)
Sat, Nov 9, 9:42 AM
Unknown Object (File)
Sat, Nov 9, 9:42 AM
Unknown Object (File)
Sat, Nov 9, 4:45 AM
Unknown Object (File)
Fri, Oct 18, 9:02 PM
Unknown Object (File)
Oct 5 2024, 5:20 AM
Unknown Object (File)
Oct 5 2024, 3:31 AM
Unknown Object (File)
Oct 5 2024, 1:33 AM
Subscribers

Details

Summary

This refactor is to make D9595 possible.
The intention is to fix outgoing's ownership issue when session creation fails (send forces outgoing to be mutable).
Doesn't look like a very optimal pattern, but not sure if this can be improved are is there a more graceful way to handle this - if anyone has any suggestions I am happy to change this 😄

Creating separate diff to make it easier for reviewers, actual usage is in D9595.

Depends on D9593

Test Plan

Tunnelbroker tests (most important are test_tunnelbroker_invalid_auth and test_tunnelbroker_valid_auth).

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Oct 25 2023, 5:18 AM
bartek added 1 blocking reviewer(s): michal.

It doesn't look that bad, I don't know if this can be done much better. Perhaps @michal has any ideas?

services/tunnelbroker/src/websockets/mod.rs
240–243

you could probably extract this as type alias or even a tuple struct (which could implement some traits later if needed)

pub type ErrorWithStreamHandle<S> = (session::SessionError, SplitSink<WebSocketStream<S>, Message>);

// or
pub struct ErrorWithStreamHandle<S>(session::SessionError, SplitSink<WebSocketStream<S>, Message>);

I've talked about it with @kamil in the office, and we had a few ideas but nothing really better than this.

This revision is now accepted and ready to land.Oct 27 2023, 6:51 AM