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

Configurable LLMP client timeout #1838

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Conversation

rmalmain
Copy link
Collaborator

@rmalmain rmalmain commented Feb 5, 2024

It makes the LLMP client timeout value configurable at runtime.

I was considering first making it configurable at compile time with generics to keep the same performance, but I guess it is not really useful and more flexible like this.

@@ -496,10 +496,9 @@ where
S: State + HasExecutions,
SP: ShMemProvider + 'static,
{
/// Launch the broker and the clients and fuzz
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep that docstring in any case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since it's an internal function, I moved the old documentation to the public function.
Maybe we want documentation for both?

Copy link
Member

Choose a reason for hiding this comment

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

We can never have enough documentation I guess

@@ -155,7 +155,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {

match mode.as_str() {
"broker" => {
let mut broker = llmp::LlmpBroker::new(StdShMemProvider::new()?)?;
let mut broker = llmp::LlmpBroker::new(StdShMemProvider::new()?, None)?;
Copy link
Member

Choose a reason for hiding this comment

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

Why not have a timeout here?

@@ -2039,6 +2062,12 @@ where
listeners: vec![],
exit_cleanly_after: None,
num_clients_total: 0,
#[cfg(feature = "std")]
client_timeout: if let Some(to) = client_timeout {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this should default to a time (and not just be no-timeout if None)?
Especially since no_std doesn't have that timeout to begin with..

@@ -643,4 +643,14 @@ where

Ok(())
}

/// Launch the broker and the clients and fuzz
Copy link
Member

Choose a reason for hiding this comment

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

Document that this has the default timeout, if that's what we want(?)

Copy link
Member

@domenukk domenukk Feb 5, 2024

Choose a reason for hiding this comment

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

Maybe we want one api for with, and one for without timeouts?

Also we probably should document better which of the three(or so) different timeouts this is :D

@domenukk
Copy link
Member

domenukk commented Feb 6, 2024

I'll accept this for now, nothing urgent

@domenukk domenukk merged commit 9b82af4 into main Feb 6, 2024
26 checks passed
@domenukk domenukk deleted the llmp_configurable_client_timeout branch February 6, 2024 17:35
@rmalmain
Copy link
Collaborator Author

rmalmain commented Feb 6, 2024

We can add documentation later on. I think many other things need documentation anyway.

@domenukk
Copy link
Member

domenukk commented Feb 7, 2024

The other question remaining is if we want the option to launch it without any timeouts/have new be without timeouts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants