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

docs: docs for UDS data client/server #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

STRRL
Copy link
Member

@STRRL STRRL commented Oct 13, 2022

Signed-off-by: STRRL [email protected]

append some docs for UDS part

Copy link
Member

@Andrewmatilde Andrewmatilde left a comment

Choose a reason for hiding this comment

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

THX for contribution!!!

@STRRL
Copy link
Member Author

STRRL commented Oct 13, 2022

But I am still curious that, I see there is nowhere to close the uds connection / UnixStream, does tokio/mio would close it automatically? But I also find that there is no where to implemented the Drop trait for these structs.

@Andrewmatilde
Copy link
Member

But I am still curious that, I see there is nowhere to close the uds connection / UnixStream, does tokio/mio would close it automatically? But I also find that there is no where to implemented the Drop trait for these structs.

Mio will close it automatically.

@STRRL
Copy link
Member Author

STRRL commented Oct 13, 2022

But I am still curious that, I see there is nowhere to close the uds connection / UnixStream, does tokio/mio would close it automatically? But I also find that there is no where to implemented the Drop trait for these structs.

Mio will close it automatically.

Any reference? ❤️

@Andrewmatilde
Copy link
Member

Any reference? ❤️

Everything base on unix fd all drop here. It is really hard to trace.

https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/src/std/os/fd/owned.rs.html#167

/// child, once the client connect, it would send the serialized data to the client immediately.
///
/// It would block the current thread, so it's recommended to call this method in a new thread.
/// TODO(@STRRL): graceful shutdown is not supported yet
Copy link
Member

Choose a reason for hiding this comment

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

TODO(@STRRL): graceful shutdown is not supported yet
UDS will automatic shutdown here. IMO, we do not need graceful shutdown here.

Copy link
Member Author

@STRRL STRRL Oct 17, 2022

Choose a reason for hiding this comment

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

UDS will automatic shutdown here. IMO, we do not need graceful shutdown here.

It seems the UDS server would not be reused between multiple Proxy::reload(), and it would keep aceept() inside the infinite loop, and the listener already moved into the inner scope, there is nowhere to "drop" it.

If I am correct, that's a resource leak.

Copy link
Member

Choose a reason for hiding this comment

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

I found it. It truely leaks. I agree with you . But this is not problem here. We will not meet leak only with UDS server . We leak because we spawn a blocking server.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add TODO here

Copy link
Member

@Andrewmatilde Andrewmatilde left a comment

Choose a reason for hiding this comment

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

Rest LGTM

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