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

feat(wm): add tcp event notifications #261

Closed

Conversation

Yusuf007R
Copy link
Contributor

This PR allow TCP to receive event notifications. That way if you want to make a custom input handler you do not have to connect to TCP socket and create a named pipe at the same time.

There are some things that I'm not sure how they should be handled

  1. Which TCP Errors should be manage here
  2. Is this okay at this line?

This commits allow tcp clients to receive event notifications. That way if you want to make a custom
input handler you do not have to connect to tcp socket and create a named pipe at the same time.
@LGUG2Z LGUG2Z force-pushed the master branch 3 times, most recently from ab223f5 to 5fda4a3 Compare November 3, 2022 16:34
@LGUG2Z
Copy link
Owner

LGUG2Z commented Feb 13, 2023

Finally coming back to this 😅

For 1) I think we can just log the errors for now and figure out if we need to handle them with specific actions in the future.

For 2) I think we can scope this and avoid the drop; try wrapping it in {} and it will be auto-dropped at the end of the block.

{
    let mut connections = TCP_CONNECTIONS.lock();

    connections.insert(
        addr.clone(),
        stream.try_clone().expect("stream should be cloneable"),
    );
}

@Yusuf007R
Copy link
Contributor Author

sorry i was super busy, do you still think the same about this feature?

@LGUG2Z
Copy link
Owner

LGUG2Z commented Jun 26, 2023

Yeah, try implementing those changes and see if everything still works, then I can check out the branch and do a final review.

@LGUG2Z
Copy link
Owner

LGUG2Z commented Mar 26, 2024

Gonna close this out now since we have the komorebi_client crate and I want to centralize all future IPC work around UDS.

@LGUG2Z LGUG2Z closed this Mar 26, 2024
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