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

[TECH_DEBT] - Don't Sleep When Waiting for Message #2558

Closed
bfish713 opened this issue Feb 9, 2024 · 1 comment · Fixed by #3671
Closed

[TECH_DEBT] - Don't Sleep When Waiting for Message #2558

bfish713 opened this issue Feb 9, 2024 · 1 comment · Fixed by #3671

Comments

@bfish713
Copy link
Collaborator

bfish713 commented Feb 9, 2024

What is this task and why do we need to work on it?

The network task does this:

    let direct_handle = async_spawn(async move {
        loop {
            let msgs = match network.recv_msgs(TransmitType::Direct).await {
                Ok(msgs) => Messages(msgs),
                Err(err) => {
                    error!("failed to receive direct messages: {err}");

                    // return zero messages so we sleep and try again
                    Messages(vec![])
                }
            };
            if msgs.0.is_empty() {
                async_sleep(Duration::from_millis(100)).await;
            } else {
                state.handle_messages(msgs.0).await;
            }
        }
    });

Ideally recv_messages would not be able to return an empty vector.
Sadly the web server implementation uses locked vectors for it's message queues so we can't simply await for the next message, we have to periodically check the queue.

What work will need to be done to complete this task?

Either simply remove the sleep case and just await in a loop when we get rid of the webserver, or fix the webserver impl to use channels where it should.

Are there any other details to include?

No response

What are the acceptance criteria to close this issue?

No sleeps in this code

Branch work will be merged to (if not the default branch)

No response

@QuentinI
Copy link
Contributor

@rob-maron @bfish713 stumbled upon this one, can we remove the sleep now?

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

Successfully merging a pull request may close this issue.

2 participants