Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

abstract tx5 backend in prep for m̶o̶c̶k̶ mem backend #105

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

neonphog
Copy link
Collaborator

@neonphog neonphog commented Oct 9, 2024

  • Actual mock backend PR to follow.
  • There are a couple apis that aren't great encapsulation for the abstraction at this level, but in the name of expediency, I've just called them out in the code for now to be addressed at a later date.
  • I'd love for the mock config to be configurable all the way up into the holochain conductor config, so I went with a generic serde_json::Value because those items will not be relevant to any other backends, but we need a shared abstraction. I'm happy to have feedback / suggestions of different ways to do this.

/// The Go Pion-based backend.
GoPion,

#[cfg(feature = "backend-webrtc-rs")]
Copy link
Member

Choose a reason for hiding this comment

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

How is this doing? Is it coming along at all or still not production ready?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wish I knew, but it doesn't seem to be the appropriate time to evaluate it. Once we have everything stable it'd be great to just go straight for the implementation so we can do a direct A/B comparison through our CI and manual usage.

impl Default for BackendModule {
#[allow(unreachable_code)]
fn default() -> Self {
#[cfg(feature = "backend-go-pion")]
Copy link
Member

Choose a reason for hiding this comment

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

Are these already prevented from being enabled together?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are currently, but when we get to implementing it, I'd actually like them to not be exclusive, hence the fallback pattern in this default constructor. Not that we'd compile very often with both of them, but it would fit better into rust's features being additive paradigm.


/// Get connection statistics.
// TODO - this isn't good encapsulation
fn get_stats(&self) -> tx5_connection::ConnStats;
Copy link
Member

Choose a reason for hiding this comment

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

I would like it if we got a structure back based on the currently enabled back end. I was fine with tx5 vs tx2 just returning different data and I think the same is still true with go-pion vs webrtc-rs vs mock. Even if it is different based on the enabled back end, that's what I'd like to see presented up at the conductor level.

Maybe an enum could work and then the encapsulation is reasonable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure that could work. Alternately, we could have this return serde_json::Value and have concrete types exposed that can be transcoded into if you know the backend you're using.

For now, I was just going to use this and the mock backend will return a blank ConnStats...


/// Returns `true` if we successfully connected over webrtc.
// TODO - this isn't good encapsulation
fn is_using_webrtc(&self) -> bool;
Copy link
Member

Choose a reason for hiding this comment

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

What would the right interface look like here? Something like an inner that let's you get at the real connection and you have to know which ones it makes sense to ask about the use of webrtc as the caller?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is this function for? Is it to check if it's connected or to check if it's using WebRTC? If it's the former then wouldn't is_connected be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the latter. "Connections" can communicate directly through sbd, then they attempt to upgrade to a webrtc connection. So is_using_webrtc() returns true once/if that upgrade is successful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would the right interface look like here?

Yeah, that's still TBD. Does it even make sense to have this call be separate? Maybe it should just be part of the stats call, although that would just defer the problem, because there's an encapsulation question about that too : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that's a different topic, but why does tx5 need to know whether it's connected indirectly through sbd or directly through webrtc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I remember, it's informational or for unit tests. But I still submit this is something that doesn't have to be sorted out to merge this PR, since I'm not changing any functionality here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked out of curiosity, it wasn't meant as a subtle push to change it. I prefer being explicit about pushes ;-) Fine by me to keep as is.

}
}

struct GoWaitCon(
Copy link
Member

Choose a reason for hiding this comment

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

When the inner fields aren't trivial, I'd prefer named fields

Copy link
Member

Choose a reason for hiding this comment

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

Although I see these get extracted into named fields right away in the function below... So I guess as long as the code is written that way, you don't actually have to work too hard to work out what the 0, 1 and 2 are

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's fair. It started out as one, then gradually went up to three. I should switch it to the struct form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done: 681fd98

@@ -81,6 +91,8 @@ impl Default for Config {
backoff_start: std::time::Duration::from_secs(5),
backoff_max: std::time::Duration::from_secs(60),
preflight: None,
backend_module: crate::backend::BackendModule::default(),
Copy link
Member

Choose a reason for hiding this comment

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

Should this only be compile time configurable? Just thinking about writing tests against this, we'd have to build and test one way to run most tests and then build and test again with different features to use mocking

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your question seems to answer why I wouldn't want to switch this to compile-time configuration. This way, most of the tests can use the true backend, but I can also have some which exercise the mock without having to have multiple top-level calls to cargo test with different features.

Comment on lines +14 to +38
/// Backend modules usable by tx5.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum BackendModule {
#[cfg(feature = "backend-go-pion")]
/// The Go Pion-based backend.
GoPion,

#[cfg(feature = "backend-webrtc-rs")]
/// The Webrtc-RS-based backend.
WebrtcRs,

/// The mock backend.
Mock,
}

impl Default for BackendModule {
#[allow(unreachable_code)]
fn default() -> Self {
#[cfg(feature = "backend-go-pion")]
return Self::GoPion;
#[cfg(feature = "backend-webrtc-rs")]
return Self::WebrtcRs;
Self::Mock
}
}
Copy link
Contributor

@cdunster cdunster Oct 10, 2024

Choose a reason for hiding this comment

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

This could also be achieved by deriving Default. The Mock variant becomes a bit ugly with listing all the features but I still think it's clearer (opinion) and removes the need for #[allow(unreachable_code)] 🙂

Suggested change
/// Backend modules usable by tx5.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum BackendModule {
#[cfg(feature = "backend-go-pion")]
/// The Go Pion-based backend.
GoPion,
#[cfg(feature = "backend-webrtc-rs")]
/// The Webrtc-RS-based backend.
WebrtcRs,
/// The mock backend.
Mock,
}
impl Default for BackendModule {
#[allow(unreachable_code)]
fn default() -> Self {
#[cfg(feature = "backend-go-pion")]
return Self::GoPion;
#[cfg(feature = "backend-webrtc-rs")]
return Self::WebrtcRs;
Self::Mock
}
}
/// Backend modules usable by tx5.
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum BackendModule {
#[cfg(feature = "backend-go-pion")]
#[default]
/// The Go Pion-based backend.
GoPion,
#[cfg(feature = "backend-webrtc-rs")]
#[default]
/// The Webrtc-RS-based backend.
WebrtcRs,
#[cfg_attr(
not(any(feature = "backend-go-pion", feature = "backend-webrtc-rs")),
default
)]
/// The mock backend.
Mock,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My hope was to make the backend features non-exclusive, hence the fallback returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens in Callums suggestion when you compile with both features? Compiler error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it would be a compiler error for multiple defined defaults. You could fix it with more cfg_attr like setting WebrtcRs to only default if go-pion isn't enabled:

#[cfg(feature = "backend-webrtc-rs")]
#[cfg_attr(not(feature = "backend-go-pion"), default)]
/// The Webrtc-RS-based backend.
WebrtcRs,

but I can imagine that this starts to look unruly as we possibly add more backends.


/// Returns `true` if we successfully connected over webrtc.
// TODO - this isn't good encapsulation
fn is_using_webrtc(&self) -> bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this function for? Is it to check if it's connected or to check if it's using WebRTC? If it's the former then wouldn't is_connected be better?

}

/// Trait-object version of backend connection.
pub type DynBackCon = Arc<dyn BackCon + 'static + Send + Sync>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use trait objects instead of generics?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Type-erasure. The associated types (or sub-generics) quickly get ridiculous with the stack of traits like this.

Comment on lines 48 to 53
let (con, con_recv) = match (con, con_recv) {
(_, None) | (None, _) => {
return Err(std::io::Error::other("already awaited"))
}
(Some(con), Some(con_recv)) => (con, con_recv),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This code could be reduced a bit with zip and ok_or but I get that this is a functional approach which isn't for everyone 🙂

Suggested change
let (con, con_recv) = match (con, con_recv) {
(_, None) | (None, _) => {
return Err(std::io::Error::other("already awaited"))
}
(Some(con), Some(con_recv)) => (con, con_recv),
};
let (con, con_recv) = con
.zip(con_recv)
.ok_or(std::io::Error::other("already awaited"))?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. I'll decline the functional change, but I was able to compact the match a bit by flipping the order and using a catch all: 681fd98

crates/tx5/src/backend/go_pion.rs Outdated Show resolved Hide resolved
neonphog and others added 2 commits October 10, 2024 11:39
Co-authored-by: Callum Dunster <cdunster@users.noreply.github.com>
@neonphog neonphog changed the title abstract tx5 backend in prep for mock backend abstract tx5 backend in prep for ~~mock~~ mem backend Oct 11, 2024
@neonphog neonphog changed the title abstract tx5 backend in prep for ~~mock~~ mem backend abstract tx5 backend in prep for m̶o̶c̶k̶ mem backend Oct 11, 2024
@neonphog neonphog merged commit 60c2321 into main Oct 14, 2024
7 checks passed
@neonphog neonphog deleted the abstract-backend branch October 14, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants